-
Notifications
You must be signed in to change notification settings - Fork 4k
sql/jsonpath: support index acceleration with AnyKey (*) at the chain end
#156416
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
Conversation
dc1e0f2 to
e33cbda
Compare
17c2b26 to
cc15ec0
Compare
yuzefovich
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, looks good to me, just some nits.
@yuzefovich reviewed 5 of 5 files at r1, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @ZhouXing19)
pkg/util/jsonpath/path.go line 319 at r1 (raw file):
// isSupportedPathPattern returns true if the given paths matches one of // the following patterns, which can be supported by the inverted index: // - keychain mode: $.[key|wildcard].[key|wildcard]...
nit: let's update the comment to include optional AnyKey at the end.
pkg/util/jsonpath/path.go line 592 at r1 (raw file):
// In these 2 cases, after "b", there is still "c" // as the next key, so "b" is not an end key. if _, isAnyKey := pathComponents[j].(AnyKey); isAnyKey {
Given the check in isSupportedPathPattern, isAnyKey can only be true when j == len(pathComponents)-1, right? Perhaps we should only check that specific value of j?
pkg/sql/logictest/testdata/logic_test/jsonb_path_exists_index_acceleration line 560 at r1 (raw file):
SELECT a FROM json_tab@foo_inv WHERE jsonb_path_exists(b, '$.a ? (@.b == $x)', '{"x": "c"}') ORDER BY a; subtest anykey
nit: we usually add an empty line after subtest directives.
cc15ec0 to
db0712a
Compare
…in end We now support index accelerating `jsonb_path_exists` filters with json path expression that ends with an AnyKey (`*`). Note that the AnyKey is allowed only at the end of the expression. I.e. the following are not allowed: ``` $.a.*.b $.a.b.*.* ``` Release note (sql change): We now support index accelerating `jsonb_path_exists` filters with json path expression that ends with an AnyKey (`*`).
db0712a to
c22a945
Compare
ZhouXing19
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @yuzefovich)
pkg/util/jsonpath/path.go line 319 at r1 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
nit: let's update the comment to include optional AnyKey at the end.
Done.
pkg/util/jsonpath/path.go line 592 at r1 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
Given the check in
isSupportedPathPattern,isAnyKeycan only betruewhenj == len(pathComponents)-1, right? Perhaps we should only check that specific value ofj?
Yes, good idea! Done.
yuzefovich
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the backport label - it seems like a nice improvement to the new feature with low risk, let's see whether Michael agrees.
@yuzefovich reviewed 2 of 2 files at r2, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @ZhouXing19)
|
TFTR! |
152448: changefeedccl: protect system tables with their own pts record r=andyyang890 a=aerfrei Previously, we were relying on each pts record that protects a user table to also protect system tables. This would mean that we'd protect the system tables once for each table. Now, we have a dedicated pts record for system tables that protects back to the highwater and do not include the system table protections in the table specific records. We only protect the system tables once. Fixes: #152446 Epic: CRDB-1421 Release note: None 155701: jobsprotectedtsccl: move to jobsprotectedts r=msbutler a=stevendanna These tests are now allowed to live in the jobs package so the jobsccl package is no longer require. Epic: none Release note: None 156190: changefeedccl: update timestamp on event descriptor cache hits r=andyyang890 a=aerfrei Before, in some cases we could replan a CDC query with an old timestamp from the event descriptor cache, which would try to read the database descriptor at an old potentially garbage collected timestamp. Now, we make sure the event descriptor cache always gives events descriptors with an up to date timestamp. Epic: none Fixes: #156091 Release note (bug fix): A bug where changefeeds using CDC queries could sometimes unexpectedly fail after a schema change with a descriptor retrieval error has been fixed. 156416: sql/jsonpath: support index acceleration with AnyKey (`*`) at the chain end r=ZhouXing19 a=ZhouXing19 fixes: #156340 We now support index accelerating `jsonb_path_exists` filters with json path expression that ends with an AnyKey (`*`). Note that the AnyKey is allowed only at the end of the expression. I.e. the following are not allowed: ``` $.a.*.b $.a.b.*.* ``` Release note (sql change): We now support index accelerating `jsonb_path_exists` filters with json path expression that ends with an AnyKey (`*`). Co-authored-by: Aerin Freilich <aerin.freilich@cockroachlabs.com> Co-authored-by: Steven Danna <danna@cockroachlabs.com> Co-authored-by: ZhouXing19 <zhouxing@uchicago.edu>
|
This PR was included in a batch that successfully built, but then failed to merge into master (it was a non-fast-forward update). It will be automatically retried. |
|
Based on the specified backports for this PR, I applied new labels to the following linked issue(s). Please adjust the labels as needed to match the branches actually affected by the issue(s), including adding any known older branches. Issue #156340: branch-release-25.4. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
fixes: #156340
We now support index accelerating
jsonb_path_existsfilters with json path expression that ends with an AnyKey (*).Note that the AnyKey is allowed only at the end of the expression. I.e. the following are not allowed:
Release note (sql change): We now support index accelerating
jsonb_path_existsfilters with json path expression that ends with an AnyKey (*).