Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

jsonpath: add jsonpath conditional evaluation #143097

Merged
merged 2 commits into from
Mar 20, 2025

Conversation

normanchenn
Copy link
Member

@normanchenn normanchenn commented Mar 18, 2025

jsonpath: add jsonpath conditional evaluation

This commit adds support for evaluating jsonpath conditionals.
Conditionals have the structure of $ ? (predicate), and will filter
the current json objects based on whether they satisfy the predicate
expression. Additionally, this commit introduces @, which represents
the current json object so filters can be used to reference the object
being evaluated.

Informs: #142610
Epic: None
Release note (sql change): Add jsonpath filters, which take on the form
$ ? (predicate), allowing results to be filtered.

jsonpath/eval: add unwrap parameter to jsonpath.eval

Previously, jsonpath conditionals/filters would unwrap infinitely. For
example, SELECT jsonb_path_query('{"a": [[[[{"b": 1}]]]]}', '$.a ? (@.b == 1)');
would return an entry, when postgres would not. This commit adds an
unwrap boolean parameter to eval, which allows for control over when
to stop unwrapping json arrays. Additionally, this commit updates
eval to take in a json.JSON rather than a []json.JSON, simplifying
the evaluation logic.

Epic: None
Release note: None

@normanchenn normanchenn self-assigned this Mar 18, 2025
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@normanchenn normanchenn marked this pull request as ready for review March 19, 2025 13:48
@normanchenn normanchenn requested a review from yuzefovich March 19, 2025 13:48
@normanchenn normanchenn mentioned this pull request Mar 19, 2025
18 tasks
This commit adds support for evaluating jsonpath conditionals.
Conditionals have the structure of `$ ? (predicate)`, and will filter
the current json objects based on whether they satisfy the predicate
expression. Additionally, this commit introduces `@`, which represents
the current json object so filters can be used to reference the object
being evaluated.

Epic: None
Release note (sql change): Add jsonpath filters, which take on the form
`$ ? (predicate)`, allowing results to be filtered.
@normanchenn normanchenn force-pushed the norman/conditionals branch from 4fa8d4e to b2af3fd Compare March 19, 2025 14:05
Copy link
Member

@yuzefovich yuzefovich left a comment

Choose a reason for hiding this comment

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

Very nice! I only have a handful of nits.

Reviewed 9 of 9 files at r1, 7 of 7 files at r2, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @normanchenn)


pkg/util/jsonpath/eval/array.go line 19 at r2 (raw file):

	if jsonValue.Type() == json.ArrayJSONType {
		// Do not evaluate any paths, just unwrap the current target.
		return ctx.unwrapCurrentTargetAndEval(nil, jsonValue, !ctx.strict)

nit: add /* jsonPath */ after nil.


pkg/util/jsonpath/eval/array.go line 72 at r2 (raw file):

		}
		return agg, nil
	} else {

nit: with error cases like this it's a bit cleaner to do the negative check first, return an error, and then have the happy logic outside of the if block, i.e. something like

if jsonValue.Type() != json.ArrayJSONType && ctx.strict {
  < return error >
}
< do the happy path logic >

pkg/util/jsonpath/parser/testdata/jsonpath line 359 at r1 (raw file):

$.a[*] ? (@.b > 100)
----
$."a"[*]?((@."b" > 100)) -- normalized!

nit: the postgres always uses single parenthesis (even if the original had multiple), should we leave a TODO for this here?


pkg/util/jsonpath/eval/operation.go line 141 at r2 (raw file):

	op jsonpath.Operation, jsonValue json.JSON, unwrapRight bool,
) (jsonpathBool, error) {
	left, err := ctx.evalAndUnwrapResult(op.Left, jsonValue, true)

nit: a quick comment for why we're always unwrapping on the left would be helpful.


pkg/util/jsonpath/eval/filter.go line 17 at r2 (raw file):

) ([]json.JSON, error) {
	if unwrap && current.Type() == json.ArrayJSONType {
		return ctx.unwrapCurrentTargetAndEval(p, current, false)

nit: add /* unwrap */ after false.

It is a common convention to specify the name of parameters when unnamed constants (like false or nil) are passed in the production code. Additionally, it might also be helpful to do the same after !ctx.strict in all callsites, to make it more clear too.


pkg/util/jsonpath/eval/eval.go line 122 at r2 (raw file):

) ([]json.JSON, error) {
	if jsonValue.Type() != json.ArrayJSONType {
		return nil, errors.Newf("unwrapCurrentTargetAndEval can only be applied to an array")

nit: should errors.Newf be assertion failures here and in executeAnyItem?

Previously, jsonpath conditionals/filters would unwrap infinitely. For
example, `SELECT jsonb_path_query('{"a": [[[[{"b": 1}]]]]}', '$.a ? (@.b == 1)');`
would return an entry, when postgres would not. This commit adds an
unwrap boolean parameter to `eval`, which allows for control over when
to stop unwrapping json arrays. Additionally, this commit updates
`eval` to take in a `json.JSON` rather than a `[]json.JSON`, simplifying
the evaluation logic.

Epic: None
Release note: None
@normanchenn normanchenn force-pushed the norman/conditionals branch from b2af3fd to 743d8bd Compare March 19, 2025 20:17
Copy link
Member Author

@normanchenn normanchenn left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @yuzefovich)


pkg/util/jsonpath/parser/testdata/jsonpath line 359 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: the postgres always uses single parenthesis (even if the original had multiple), should we leave a TODO for this here?

Yep, I added a TODO in pkg/util/jsonpath/operation.go:Operation.String() about this in a previous commit.

Copy link
Member

@yuzefovich yuzefovich left a comment

Choose a reason for hiding this comment

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

Thanks! :lgtm:

Reviewed 7 of 7 files at r4, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @normanchenn)

@normanchenn
Copy link
Member Author

TFTR!

bors r+

@craig craig bot merged commit 571bcac into cockroachdb:master Mar 20, 2025
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants