Support JSON_EXISTS, JSON_VALUE and JSON_QUERY functions by kasiafi · Pull Request #9831 · trinodb/trino

kasiafi · 2021-11-02T13:14:58Z

No description provided.

core/trino-parser/src/main/antlr4/io/trino/sql/parser/SqlBase.g4

core/trino-parser/src/main/antlr4/io/trino/jsonpath/JsonPath.g4

martint · 2022-04-26T20:38:20Z

core/trino-parser/src/main/java/io/trino/sql/tree/JsonExists.java

In that case, the rewriter should create the JsonExists node with the same location as the original one, and we should remove the location-less constructor. Same for the other ones.

martint · 2022-04-26T20:51:42Z

core/trino-main/src/main/java/io/trino/operator/scalar/json/JsonOutputFunctions.java

I'd call this serialize

martint · 2022-04-26T21:42:35Z

core/trino-main/src/main/java/io/trino/operator/scalar/json/JsonParametersToArrayHelper.java

What's this class for? The name is too vague and the method below is not very clear. I'm thinking of whether it should be renamed or placed elsewhere.

I added a javadoc, and renamed the class to ParameterUtil.

martint · 2022-04-26T22:03:11Z

core/trino-main/src/main/java/io/trino/sql/analyzer/ExpressionAnalyzer.java

When would this happen?

When function is not registered, which shouldn't happen. Should I remove the try-catch?

martint · 2022-04-26T22:06:42Z

core/trino-main/src/main/java/io/trino/sql/analyzer/ExpressionAnalyzer.java

Call .build() here directly and declare the variable as List<Type>. No need to have an explicit variable to hold the intermediate.

martint · 2022-04-26T22:09:25Z

core/trino-main/src/main/java/io/trino/sql/analyzer/JsonPathAnalyzer.java

Remove or add a TODO comment.

martint · 2022-04-26T22:10:30Z

core/trino-main/src/main/java/io/trino/sql/planner/JsonPathEvaluator.java

This should not be in the "planner" package, but in the io.trino.json package.

martint · 2022-04-26T22:14:22Z

core/trino-main/src/main/java/io/trino/sql/planner/JsonPathEvaluator.java

This seems backward. The fixed part should be the path (i.e., the "program" or "IR"), the functions, etc. The variable part should be the json over which the path is being evaluated.

That will allow caching resolutions and other similar decisions and attach them to the corresponding IR nodes.

martint · 2022-04-26T22:17:53Z

core/trino-main/src/main/java/io/trino/sql/planner/JsonPathEvaluator.java

Where is the operator being cached? Ideally, it should be attached to the node (via a map of node->operator) to avoid having to re-resolve it every time.

The operators are cached in CachingResolver.

kasiafi · 2022-05-06T16:37:58Z

@martint I refactored operator caching according to our discussion. Please take a look at the last commit.

martint · 2022-05-24T18:20:53Z

core/trino-main/src/main/java/io/trino/json/JsonPathInvocationContext.java

It's not obvious by looking at this why it's safe to skip re-initializing the evaluator if it's already set. Technically, and from the JsonPathInvocationContext's perspective, the path could be different. The fact that it's not, is a side effect of how the JSON constructs in SQL work (the path is constant) and how they are mapped to functions. But that knowledge is too far removed from this site.

I don't think this class is needed. Instead, the functions should initialize and hold on to the JsonPathEvaluator instance upon first invocation (i.e., wherever they are calling initializeIfNecessary)

I think that JsonPathInvocationContext is needed. It is passed through the instanceFactory and this way an instance of JsonPathInvocationContext persists through multiple rows processed by the JSON-funciton.

We can't skip it and just pass JsonPathEvaluator as the context object, because we can create the JsonPathEvaluator only after the function is invoked with certain arguments (including path). The context object already exists at that moment (it comes as one of the arguments to the function).

Yes, you're right. This is the state object for the (stateful) function. However, it could be changed to be just that -- a state object, and leave all logic to be done in the caller.

Let `handleSubqueries` find subqueries nested under `Expression` nodes, linked via children not being `Expression`.

`json_extract` and `json_extract_scalar` are old JSON-processing functions which have very limited capabilities, and are not compliant with the spec. However, they have simple lightweight implementation, optimized for the use case. `json_query` and `json_value` are new spec-compliant JSON-processing functions, which support the complete specification for JSON path. Their implementation is much more complicated, resulting both from the scope of the supported feature, and the "streaming" semantics. These benchmarks are to compare both implementations applied to the common use-case. Benchmark json_extract vs json_query: Benchmark (depth) Mode Cnt Score Error Units BenchmarkJsonFunctions.benchmarkJsonExtractFunction 1 avgt 15 813.030 ± 12.263 ns/op BenchmarkJsonFunctions.benchmarkJsonExtractFunction 3 avgt 15 939.202 ± 94.621 ns/op BenchmarkJsonFunctions.benchmarkJsonExtractFunction 6 avgt 15 1004.352 ± 12.945 ns/op BenchmarkJsonFunctions.benchmarkJsonQueryFunction 1 avgt 15 1136.371 ± 17.502 ns/op BenchmarkJsonFunctions.benchmarkJsonQueryFunction 3 avgt 15 1399.780 ± 27.967 ns/op BenchmarkJsonFunctions.benchmarkJsonQueryFunction 6 avgt 15 1666.810 ± 29.572 ns/op Benchmark json_extract_scalar vs json_value: Benchmark (depth) Mode Cnt Score Error Units BenchmarkJsonFunctions.benchmarkJsonExtractScalarFunction 1 avgt 15 644.762 ± 9.195 ns/op BenchmarkJsonFunctions.benchmarkJsonExtractScalarFunction 3 avgt 15 720.244 ± 13.288 ns/op BenchmarkJsonFunctions.benchmarkJsonExtractScalarFunction 10 avgt 15 928.069 ± 17.623 ns/op BenchmarkJsonFunctions.benchmarkJsonValueFunction 1 avgt 15 811.158 ± 14.694 ns/op BenchmarkJsonFunctions.benchmarkJsonValueFunction 3 avgt 15 1042.795 ± 40.555 ns/op BenchmarkJsonFunctions.benchmarkJsonValueFunction 10 avgt 15 1663.328 ± 27.320 ns/op

martint · 2022-05-26T18:43:37Z

core/trino-main/src/main/java/io/trino/json/CachingResolver.java

Why not flatten the caches an use a composite key made of IrPathNode ref, left type and right type?

Then one node could take too much space.

That should be ok, especially if there's a case where a single node sees many combinations of types.

It would also make the code simpler. If later we find that this is not sufficient, we can think of better caching and eviction strategies, or JIT-style codegen/execution

The cache is now flattened.

findepi · 2022-05-28T06:34:27Z

Nice!
it took a while, so the more happy it landed. 🎉

cla-bot bot added the cla-signed label Nov 2, 2021

kasiafi force-pushed the 286jsonFeature branch 6 times, most recently from fe236ff to 70e48c9 Compare November 8, 2021 18:54

kasiafi force-pushed the 286jsonFeature branch 2 times, most recently from 0acac64 to e1f3ccd Compare November 12, 2021 17:19

kasiafi changed the title ~~Support JSON_EXISTS, JSON_VALUE and JSON_QUERY functions in grammar and AST~~ Support JSON_EXISTS, JSON_VALUE and JSON_QUERY functions in grammar, AST and Analyzer Nov 29, 2021

kasiafi force-pushed the 286jsonFeature branch 5 times, most recently from 5fa3108 to 1866670 Compare November 30, 2021 13:23

kasiafi force-pushed the 286jsonFeature branch 7 times, most recently from a53e24a to 5b355b5 Compare December 14, 2021 14:42

kasiafi force-pushed the 286jsonFeature branch 3 times, most recently from 4a4b801 to a17b750 Compare December 16, 2021 07:45

martint reviewed Dec 17, 2021

View reviewed changes

core/trino-parser/src/main/antlr4/io/trino/sql/parser/SqlBase.g4 Outdated Show resolved Hide resolved

core/trino-parser/src/main/antlr4/io/trino/jsonpath/JsonPath.g4 Outdated Show resolved Hide resolved

kasiafi force-pushed the 286jsonFeature branch 4 times, most recently from b1c768f to b5d5f26 Compare December 30, 2021 09:43

kasiafi force-pushed the 286jsonFeature branch from 6afbefa to a6fb1c8 Compare April 22, 2022 11:03

findepi mentioned this pull request Apr 22, 2022

Process function path #11476

Merged

kasiafi force-pushed the 286jsonFeature branch from a6fb1c8 to 1701267 Compare April 23, 2022 13:05

martint reviewed Apr 26, 2022

View reviewed changes

kasiafi force-pushed the 286jsonFeature branch 2 times, most recently from b5341a7 to 04d22e4 Compare May 6, 2022 13:23

kasiafi force-pushed the 286jsonFeature branch 3 times, most recently from 32c0f72 to 4838a88 Compare May 19, 2022 13:09

martint reviewed May 24, 2022

View reviewed changes

kasiafi added 8 commits May 26, 2022 16:48

Add JSON_EXISTS, JSON_VALUE and JSON_QUERY to grammar and AST

ae0db1b

Analyze JSON_EXISTS, JSON_VALUE and JSON_QUERY functions

238460a

Handle subqueries through non-Expressions

59b45c3

Let `handleSubqueries` find subqueries nested under `Expression` nodes, linked via children not being `Expression`.

Plan and execute JSON_EXISTS, JSON_VALUE and JSON_QUERY functions

b4d1b13

Refactor JSON functions to use parameter array

1b19e35

Pass parameters row as Block

b37b674

Benchmark binary operator in JSON path

350d8e0

kasiafi force-pushed the 286jsonFeature branch from 4838a88 to 7e0d34c Compare May 26, 2022 15:10

martint reviewed May 26, 2022

View reviewed changes

kasiafi force-pushed the 286jsonFeature branch from 7e0d34c to 8834f41 Compare May 26, 2022 18:58

Cache resolved operators for JSON path binary expressions

dd31ddf

kasiafi force-pushed the 286jsonFeature branch from 8834f41 to dd31ddf Compare May 27, 2022 09:08

martint approved these changes May 27, 2022

View reviewed changes

kasiafi mentioned this pull request May 27, 2022

Release notes for 383 #12556

Closed

colebow mentioned this pull request May 31, 2022

Add Trino 383 release notes #12565

Merged

kasiafi mentioned this pull request Jun 6, 2022

Add support for SQL:2016 JSON features #9081

Open

Praveen2112 mentioned this pull request Mar 16, 2023

Function json_query not registered #16591

Closed

Conversation

kasiafi commented Nov 2, 2021

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kasiafi commented May 6, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

findepi commented May 28, 2022

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants