Add JsonStringArrayExtractScalar function #22379

Dith3r · 2024-06-13T09:52:41Z

Description

Optimize common case of transform(cast(json_parse), json_extract_scalar) to be single and more performant call of JsonStringArrayExtractScalar.

Example query:

WITH t AS (SELECT a.id as id, a.data AS data FROM table_a AS a) 
SELECT b.data FROM table_b AS b, t AS a 
WHERE b.id = a.id AND  
CASE WHEN b.data <> 'second' THEN contains(transform(cast(json_parse(a.data) as array<json>), (x) -> json_extract_scalar("x", '$.name')), b.data) 
ELSE false END;

With change:

     │   CPU: 8.58s (94.13%), Scheduled: 8.93s (69.96%), Blocked: 1.89s (94.59%), Output: 3492632 rows (34.65MB)

Without:

     │   CPU: 16.49s (95.06%), Scheduled: 17.24s (82.05%), Blocked: 2.81s (92.72%), Output: 3492632 rows (34.65MB)

Additional context and related issues

Release notes

( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
(X) Release notes are required, with the following suggested text:

# General
* Improve performance of parsing JSON array and then extracting scalar value for each array entry

core/trino-main/src/main/java/io/trino/metadata/SystemFunctionBundle.java

core/trino-main/src/main/java/io/trino/operator/scalar/JsonStringArrayExtractScalar.java

sopel39 · 2024-06-14T14:40:34Z

core/trino-main/src/main/java/io/trino/operator/scalar/JsonStringArrayExtractScalar.java

+ continue;
+ }
+ JsonParser jsonParser = parser.readValueAsTree().traverse();
+ jsonParser.nextToken();


shouldn't you check token is not null?

all requierd checks are within extractor.

...rino-main/src/main/java/io/trino/sql/ir/optimizer/rule/SpecializeTransformWithJsonParse.java

core/trino-main/src/test/java/io/trino/operator/scalar/TestJsonStringArrayExtractScalar.java

core/trino-main/src/main/java/io/trino/operator/scalar/JsonStringArrayExtractScalar.java

core/trino-main/src/test/java/io/trino/operator/scalar/TestJsonStringArrayExtractScalar.java

core/trino-main/src/main/java/io/trino/operator/scalar/JsonStringArrayExtractScalar.java

sopel39 · 2024-06-17T14:45:56Z

core/trino-main/src/main/java/io/trino/operator/scalar/JsonStringArrayExtractScalar.java

+ append(path.getScalarExtractor().extract(jsonParser), elementBuilder);
+ }
+ catch (JsonParseException e) {
+ append(null, elementBuilder);


How does that comply with javadoc: https://trino.io/docs/current/functions/json.html#json_extract_scalar
What happens if malformed value is used? Maybe javadoc should be changed?

It depends, if the malformed value is json, is scalar or array/object.

core/trino-main/src/main/java/io/trino/operator/scalar/JsonStringArrayExtractScalar.java

...rino-main/src/main/java/io/trino/sql/ir/optimizer/rule/SpecializeTransformWithJsonParse.java

core/trino-main/src/test/java/io/trino/operator/scalar/TestJsonStringArrayExtractScalar.java

sopel39

lgtm % comments

core/trino-main/src/main/java/io/trino/operator/scalar/JsonStringArrayExtractScalar.java

sopel39 · 2024-06-18T09:42:41Z

core/trino-main/src/test/java/io/trino/operator/scalar/TestJsonStringArrayExtractScalar.java

+ void testEmptyArrayExtract()
+ {
+ Result result = extract("[]", "$.name");
+ assertThat(result.value()).isEqualTo(List.of());


isEqualTo(List.of()); -> isEmpty()

result.value is Object and it will require to cast -> assertThat((List<?>) result.value()).isEmpty();

core/trino-main/src/test/java/io/trino/operator/scalar/TestJsonStringArrayExtractScalar.java

core/trino-main/src/test/java/io/trino/sql/query/QueryAssertions.java

core/trino-main/src/test/java/io/trino/operator/scalar/TestJsonStringArrayExtractScalar.java

core/trino-main/src/test/java/io/trino/sql/query/QueryAssertions.java

Optimzie common case of transform(cast(json_parse(varchar_col) as array<json>), json -> json_extract_scalar(json, jsonPath)) case to be single and more performant call of JsonStringArrayExtract.

martint · 2024-06-20T16:15:07Z

core/trino-main/src/test/java/io/trino/operator/scalar/TestJsonStringArrayExtractScalar.java

+ @Test
+ void testEmptySliceExtract()
+ {
+ assertTrinoExceptionThrownBy(() -> assertions.expression("transform(cast(json_parse(a) as array<json>), x -> json_extract_scalar(\"x\", '$.name'))")


Angle brackets for parametric types are deprecated. Use parenthesis instead: array(json)

core/trino-main/src/test/java/io/trino/operator/scalar/TestJsonStringArrayExtractScalar.java

martint · 2024-06-20T16:40:03Z

core/trino-main/src/test/java/io/trino/sql/query/QueryAssertions.java

+ return new Result(full.type(), full.value, full.expression);
+ }
+
+ private static Optional<Expression> extractExpressionWithBindings(Plan plan)


What's the purpose of this method and the one below?

To get final expression how value was produced.

martint · 2024-06-20T17:09:50Z

core/trino-main/src/test/java/io/trino/sql/query/QueryAssertions.java

@@ -883,7 +906,7 @@ public ExpressionAssert assertThat()
 .withRepresentation(ExpressionAssert.TYPE_RENDERER);
 }

- public record Result(Type type, Object value) {}
+ public record Result(Type type, Object value, Optional<Expression> expression) {}


What is the optional expression?

Extracted expression from plan to check how value was produced. For example, if transformation was involved or not.

When would it be empty?

Also, this change to QueryAssertions doesn't follow the intent and spirit of that API and abstraction. It should be changed to work with AssertJ like this:

assertThat(assertions.expression("...")) .matches(<ir expression>)

cla-bot bot added the cla-signed label Jun 13, 2024

Dith3r requested a review from sopel39 June 13, 2024 09:57

Dith3r changed the title ~~Add JsonStringArrayExtract function~~ [WIP] Add JsonStringArrayExtract function Jun 13, 2024

Dith3r marked this pull request as draft June 13, 2024 09:59

Dith3r changed the title ~~[WIP] Add JsonStringArrayExtract function~~ Add JsonStringArrayExtract function Jun 13, 2024

Dith3r force-pushed the ke/json branch from dcb5ccc to 7db5c6a Compare June 13, 2024 10:01

Dith3r changed the title ~~Add JsonStringArrayExtract function~~ Add JsonStringArrayExtractScalar function Jun 13, 2024

Dith3r added the performance label Jun 13, 2024

Dith3r force-pushed the ke/json branch 3 times, most recently from a8abebd to f314662 Compare June 14, 2024 13:02

Dith3r marked this pull request as ready for review June 14, 2024 13:02

Dith3r force-pushed the ke/json branch from f314662 to d6a5f62 Compare June 14, 2024 13:59

sopel39 reviewed Jun 14, 2024

View reviewed changes

core/trino-main/src/main/java/io/trino/metadata/SystemFunctionBundle.java Show resolved Hide resolved

core/trino-main/src/main/java/io/trino/operator/scalar/JsonStringArrayExtractScalar.java Outdated Show resolved Hide resolved

sopel39 reviewed Jun 14, 2024

View reviewed changes

martint reviewed Jun 14, 2024

View reviewed changes

core/trino-main/src/main/java/io/trino/operator/scalar/JsonStringArrayExtractScalar.java Show resolved Hide resolved

Dith3r force-pushed the ke/json branch from d6a5f62 to e9c08c6 Compare June 14, 2024 19:31

Dith3r requested a review from sopel39 June 14, 2024 19:34

Dith3r commented Jun 14, 2024

View reviewed changes

core/trino-main/src/test/java/io/trino/operator/scalar/TestJsonStringArrayExtractScalar.java Outdated Show resolved Hide resolved

Dith3r force-pushed the ke/json branch from e9c08c6 to f6de1bf Compare June 14, 2024 20:02

sopel39 reviewed Jun 17, 2024

View reviewed changes

Dith3r force-pushed the ke/json branch 2 times, most recently from 66e1707 to 935b149 Compare June 18, 2024 08:26

Dith3r requested a review from sopel39 June 18, 2024 08:49

sopel39 approved these changes Jun 18, 2024

View reviewed changes

Dith3r force-pushed the ke/json branch 2 times, most recently from 504f718 to 6612645 Compare June 18, 2024 11:37

Dith3r requested a review from sopel39 June 18, 2024 11:39

Dith3r force-pushed the ke/json branch 2 times, most recently from 01cd6f3 to d794dd8 Compare June 18, 2024 12:11

sopel39 approved these changes Jun 18, 2024

View reviewed changes

Dith3r force-pushed the ke/json branch 3 times, most recently from f978d51 to 95923ec Compare June 19, 2024 13:29

sopel39 approved these changes Jun 19, 2024

View reviewed changes

core/trino-main/src/test/java/io/trino/sql/query/QueryAssertions.java Outdated Show resolved Hide resolved

core/trino-main/src/test/java/io/trino/sql/query/QueryAssertions.java Outdated Show resolved Hide resolved

Dith3r added 3 commits June 20, 2024 10:02

Add Expression to Result in ExpressionAssertProvider

f503390

Add transform function const

8ad3069

Add JsonStringArrayExtract function

117c2a6

Optimzie common case of transform(cast(json_parse(varchar_col) as array<json>), json -> json_extract_scalar(json, jsonPath)) case to be single and more performant call of JsonStringArrayExtract.

Dith3r force-pushed the ke/json branch from 95923ec to 117c2a6 Compare June 20, 2024 08:02

sopel39 merged commit 04227e3 into trinodb:master Jun 20, 2024
96 checks passed

github-actions bot added this to the 451 milestone Jun 20, 2024

martint reviewed Jun 20, 2024

View reviewed changes

colebow mentioned this pull request Jun 25, 2024

Add Trino 451 release notes #22504

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add JsonStringArrayExtractScalar function #22379

Add JsonStringArrayExtractScalar function #22379

Dith3r commented Jun 13, 2024 •

edited by sopel39

Loading

sopel39 Jun 14, 2024

Dith3r Jun 14, 2024

sopel39 Jun 17, 2024

Dith3r Jun 17, 2024

sopel39 left a comment

sopel39 Jun 18, 2024

Dith3r Jun 18, 2024 •

edited

Loading

martint Jun 20, 2024

martint Jun 20, 2024

Dith3r Jun 21, 2024

martint Jun 20, 2024 •

edited

Loading

Dith3r Jun 21, 2024

martint Jun 21, 2024

Add JsonStringArrayExtractScalar function #22379

Add JsonStringArrayExtractScalar function #22379

Conversation

Dith3r commented Jun 13, 2024 • edited by sopel39 Loading

Description

Additional context and related issues

Release notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sopel39 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dith3r Jun 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martint Jun 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dith3r commented Jun 13, 2024 •

edited by sopel39

Loading

Dith3r Jun 18, 2024 •

edited

Loading

martint Jun 20, 2024 •

edited

Loading