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

opt: index accelerate chained fetch value operators #59494

Merged
merged 2 commits into from Jan 29, 2021

Conversation

mgartner
Copy link
Collaborator

@mgartner mgartner commented Jan 27, 2021

opt: add test for JSON fetch val inverse

Release note: None

opt: index accelerate chained fetch value operators

Prior to #55316, the optimizer generated inverted index scans on indexed
JSON columns when queries had filters with chained fetch value
operators, for example j->'a'->'b' = '1'. The logic that made this
possible was found to create query plans not equivalent to the query, so
it was removed. This commit restores the ability to index accelerate
chained -> operators.

Fixes #55317

Release note (performance improvement): A bug fix included in 20.2.1 for
for the JSON fetch value operator, ->, resulted in chained ->
operators in query filters not being index accelerated, e.g.,
j->'a'->'b' = '1'. Chained -> are now index accelerated.

@mgartner mgartner requested a review from a team as a code owner January 27, 2021 22:11
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@mgartner mgartner changed the title Chained fetch val opt: index accelerate chained fetch value operators Jan 27, 2021
@mgartner mgartner force-pushed the chained-fetch-val branch 3 times, most recently from 79770e2 to 553dd54 Compare January 27, 2021 23:59
Copy link
Collaborator

@rytaft rytaft left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r1, 5 of 5 files at r2.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @mgartner and @RaduBerinde)


pkg/sql/opt/invertedidx/json_array.go, line 364 at r2 (raw file):

	// true and keys are collected and ordered by the outer-most fetch val index
	// first. For example, the expression j->'a'->'b' results in the keys
	// {"b", "a"}.

This is kind of mind-bending... but I think you've done a good job explaining with examples. I was still kind of confused for a while, though, so I think you could even go a step further in spelling it out. For example, one additional thing that might help is an addition to the function comment explaining that you'll first collect keys from the right to left, so the order of keys in the keys slice will be reversed from the order in the chain. Then you'll build up a JSON from the inside out, which will have the effect of once again reversing the order of the keys so they match the original order in the chain. That comment could show the whole transformation, e.g., j->'a'->'b' = 1 becomes j @> {"a": {"b": 1}}


pkg/sql/opt/invertedidx/json_array.go, line 386 at r2 (raw file):

		// variable or expression corresponding to the index column, then we
		// have found a valid list of keys to build an inverted expression.
		// an inverted

sentence fragment

@mgartner mgartner force-pushed the chained-fetch-val branch 2 times, most recently from 0373892 to a8d7205 Compare January 28, 2021 22:22
Copy link
Collaborator Author

@mgartner mgartner 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 (and 1 stale) (waiting on @RaduBerinde and @rytaft)


pkg/sql/opt/invertedidx/json_array.go, line 364 at r2 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

This is kind of mind-bending... but I think you've done a good job explaining with examples. I was still kind of confused for a while, though, so I think you could even go a step further in spelling it out. For example, one additional thing that might help is an addition to the function comment explaining that you'll first collect keys from the right to left, so the order of keys in the keys slice will be reversed from the order in the chain. Then you'll build up a JSON from the inside out, which will have the effect of once again reversing the order of the keys so they match the original order in the chain. That comment could show the whole transformation, e.g., j->'a'->'b' = 1 becomes j @> {"a": {"b": 1}}

Very mind-bending. On my first attempt I was sure that I'd have to iterate over the keys backwards when building the JSON object because they are in reverse order. But the outer most fetch val index becomes the inner-most JSON object key. I've updated the comment, let me know what you think.


pkg/sql/opt/invertedidx/json_array.go, line 386 at r2 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

sentence fragment

Done.

Copy link
Collaborator

@rytaft rytaft left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 5 files at r4.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @mgartner, @RaduBerinde, and @rytaft)


pkg/sql/opt/invertedidx/json_array.go, line 364 at r2 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

Very mind-bending. On my first attempt I was sure that I'd have to iterate over the keys backwards when building the JSON object because they are in reverse order. But the outer most fetch val index becomes the inner-most JSON object key. I've updated the comment, let me know what you think.

I think part of what was confusing for me was that I didn't understand that the "outer-most" key in the expression tree was the right-most key in the SQL string. Maybe just adding that fact somewhere would help.

Copy link
Collaborator Author

@mgartner mgartner 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 (and 1 stale) (waiting on @RaduBerinde and @rytaft)


pkg/sql/opt/invertedidx/json_array.go, line 364 at r2 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

I think part of what was confusing for me was that I didn't understand that the "outer-most" key in the expression tree was the right-most key in the SQL string. Maybe just adding that fact somewhere would help.

Ohhh I see. Done.

Copy link
Collaborator

@rytaft rytaft left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 5 files at r4, 1 of 1 files at r5.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @mgartner and @RaduBerinde)


pkg/sql/opt/invertedidx/json_array.go, line 364 at r2 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

Ohhh I see. Done.

Thanks!


pkg/sql/opt/invertedidx/json_array.go, line 374 at r5 (raw file):

	// Later on, we iterate forward through these keys to build a JSON object
	// from the inside-out with the inner-most value being the right scalar
	// expression. In the resulting JSON object, the outer-most JSON fetch value

[nit] maybe say "... inner-most value being the JSON scalar extracted above from the right ScalarExpr function argument"

Copy link
Collaborator Author

@mgartner mgartner 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 (and 1 stale) (waiting on @RaduBerinde and @rytaft)


pkg/sql/opt/invertedidx/json_array.go, line 374 at r5 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

[nit] maybe say "... inner-most value being the JSON scalar extracted above from the right ScalarExpr function argument"

Done.

Copy link
Collaborator

@rytaft rytaft left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 5 files at r7.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @RaduBerinde)

Prior to cockroachdb#55316, the optimizer generated inverted index scans on indexed
JSON columns when queries had filters with chained fetch value
operators, for example `j->'a'->'b' = '1'`. The logic that made this
possible was found to create query plans not equivalent to the query, so
it was removed. This commit restores the ability to index accelerate
chained -> operators.

Fixes cockroachdb#55317

Release note (performance improvement): A bug fix included in 20.2.1 for
for the JSON fetch value operator, `->`, resulted in chained `->`
operators in query filters not being index accelerated, e.g.,
`j->'a'->'b' = '1'`. Chained `->` are now index accelerated.
@mgartner
Copy link
Collaborator Author

TFTR!

bors r=rytaft

@craig
Copy link
Contributor

craig bot commented Jan 29, 2021

Build succeeded:

@craig craig bot merged commit 3f7283a into cockroachdb:master Jan 29, 2021
@mgartner mgartner deleted the chained-fetch-val branch January 29, 2021 22:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

opt: generate inverted index scans for chained JSON fetch value operators with equality expression
3 participants