Skip to content

Support more types of parameters to JSON path#12682

Merged
kasiafi merged 1 commit intotrinodb:masterfrom
kasiafi:365JsonFeatureMoreTypes
Jun 8, 2022
Merged

Support more types of parameters to JSON path#12682
kasiafi merged 1 commit intotrinodb:masterfrom
kasiafi:365JsonFeatureMoreTypes

Conversation

@kasiafi
Copy link
Copy Markdown
Member

@kasiafi kasiafi commented Jun 3, 2022

Before this change, path parameters could be only: numeric, boolean, datetime
and string values, and other values that could be implicitly coerced to varchar.

After this change, any values are accepted which can be coerced
to varchar. It is no more required that an implicit coercion is defined.

# General
* Support all types that can be cast to `varchar` as parameters to JSON path.

@cla-bot cla-bot bot added the cla-signed label Jun 3, 2022
@martint
Copy link
Copy Markdown
Member

martint commented Jun 3, 2022

For completeness, this is per section 10.12.3 - JSON value expression of the SQL specification:

  1. The declared type DT of the <value expression> VE simply contained in <JSON value expression> JVE either shall be a character string type, a binary string type, a numeric type, a datetime type, or Boolean, or shall be a <data type> the values of which can be cast to a character string type according to the Syntax Rules of Subclause 6.13, “`”.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to track these explicitly? It should be sufficient to just register a coercion via addOrReplaceExpressionsCoercion.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I considered that approach, but so far that mechanism was only used for implicit coercions. I didn't want to mix in the JSON path parameter coercions, which are part of JSON functions semantics rather than SQL type alignment.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That mechanism is for anything where the analyzer decides a cast needs to be inserted, so it should be safe to use here. The concept of whether an implicit coercion is allowed is an analysis time concern and the planner doesn't care about why a cast needs to be inserted.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact, we could say that this is an implicit coercion, just in a very narrow scope (json function parameters)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I switched to the existing mechanism (addOrReplaceExpressionsCoercion).

@kasiafi
Copy link
Copy Markdown
Member Author

kasiafi commented Jun 3, 2022

For completeness, this is per section 10.12.3 - JSON value expression of the SQL specification

Yes, that was the intention behind the code from the start. I just chose the wrong check: typeCoercion.canCoerce() instead of explicit resolution of the operator (metadata.getCoercion()).

@kasiafi kasiafi force-pushed the 365JsonFeatureMoreTypes branch 2 times, most recently from 95d4566 to ed4a3f4 Compare June 6, 2022 08:41
Before this change, the JSON path parameters could be only
numeric, boolean, datetime and string values, and other values
that could be implicitly coerced to varchar.

After this change, any values are accepted which can be coerced
to varchar. It is no more required that an implicit coercion
is defined.
@kasiafi kasiafi force-pushed the 365JsonFeatureMoreTypes branch from ed4a3f4 to 3133b2c Compare June 6, 2022 08:42
@kasiafi kasiafi requested a review from martint June 6, 2022 13:33
@kasiafi kasiafi merged commit 103b9ca into trinodb:master Jun 8, 2022
@kasiafi kasiafi mentioned this pull request Jun 8, 2022
@github-actions github-actions bot added this to the 385 milestone Jun 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

2 participants