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

sql: add FORCE_INVERTED_INDEX hint #120384

Merged
merged 4 commits into from Mar 15, 2024
Merged

Conversation

mgartner
Copy link
Collaborator

@mgartner mgartner commented Mar 12, 2024

sql/parser: add parse-no-verify test command

The parse-no-verify test command has been added to data-driven parser
tests. In addition, the error test command now asserts that parsing
the statement results in an error.

Release note: None

sql/parser: move index hint tests to new file

This is purely a mechanical movement of parser tests with index hints
into a new file.

Release note: None

sql/parser: support FORCE_INVERTED_INDEX hint

This commit adds parsing support for the FORCE_INVERTED_INDEX hint. The
hint currently has no effect.

Release note: None

sql: support FORCE_INVERTED_INDEX hint

Epic: None

Release note (sql change): The FORCE_INVERTED_INDEX hint is now
supported. This makes the optimizer prefer a query plan scan over any
inverted index of the hinted table. The query will result in an error if
no such query plan can be generated.

@mgartner mgartner requested a review from a team March 12, 2024 20:53
@mgartner mgartner requested review from a team as code owners March 12, 2024 20:53
@mgartner mgartner requested review from rytaft and removed request for a team March 12, 2024 20:53
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator

@DrewKimball DrewKimball left a comment

Choose a reason for hiding this comment

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

:lgtm: Nice!

Reviewed 4 of 4 files at r1, 2 of 2 files at r2, 3 of 3 files at r3, 8 of 8 files at r4, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @mgartner and @rytaft)


pkg/sql/parser/sql.y line 941 at r3 (raw file):

%token <str> FAILURE FALSE FAMILY FETCH FETCHVAL FETCHTEXT FETCHVAL_PATH FETCHTEXT_PATH
%token <str> FILES FILTER
%token <str> FIRST FLOAT FLOAT4 FLOAT8 FLOORDIV FOLLOWING FOR FORCE FORCE_INDEX FORCE_INVERTED_SCAN

Should it be FORCE_INVERTED_INDEX instead? You could justify either, but it'll be hard to change once we go with one.

@mgartner
Copy link
Collaborator Author

pkg/sql/parser/sql.y line 941 at r3 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

Should it be FORCE_INVERTED_INDEX instead? You could justify either, but it'll be hard to change once we go with one.

Hmmm, tough call. On the one hand FORCE_INVERTED_INDEX would be more consistent with the existing FORCE_INDEX hint. On the other hand, with FORCE_INDEX you must specify an index to force, which could be non-inverted or inverted, while this new hint will not allow specifying an index, so FORCE_INVERTED_SCAN might be better? On the third hand, in the future we could extend FORCE_INVERTED_INDEX to allow specifying an inverted index.

The `parse-no-verify` test command has been added to data-driven parser
tests. In addition, the `error` test command now asserts that parsing
the statement results in an error.

Release note: None
This is purely a mechanical movement of parser tests with index hints
into a new file.

Release note: None
@mgartner mgartner changed the title sql: add FORCE_INVERTED_SCAN hint sql: add FORCE_INVERTED_INDEX hint Mar 15, 2024
@mgartner
Copy link
Collaborator Author

pkg/sql/parser/sql.y line 941 at r3 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

Hmmm, tough call. On the one hand FORCE_INVERTED_INDEX would be more consistent with the existing FORCE_INDEX hint. On the other hand, with FORCE_INDEX you must specify an index to force, which could be non-inverted or inverted, while this new hint will not allow specifying an index, so FORCE_INVERTED_SCAN might be better? On the third hand, in the future we could extend FORCE_INVERTED_INDEX to allow specifying an inverted index.

I went with FORCE_INVERTED_INDEX. Thanks for the feedback!

This commit adds parsing support for the `FORCE_INVERTED_INDEX` hint. The
hint currently has no effect.

Release note: None
Release note (sql change): The `FORCE_INVERTED_INDEX` hint is now
supported. This makes the optimizer prefer a query plan scan over any
inverted index of the hinted table. The query will result in an error if
no such query plan can be generated.
@mgartner
Copy link
Collaborator Author

TFTR!

bors r=DrewKimball

@craig
Copy link
Contributor

craig bot commented Mar 15, 2024

@craig craig bot merged commit ae00dcb into cockroachdb:master Mar 15, 2024
22 of 36 checks passed
@mgartner mgartner deleted the force-inverted-scan branch March 16, 2024 14:36
@mgartner mgartner added backport-23.1.x Flags PRs that need to be backported to 23.1 backport-23.2.x Flags PRs that need to be backported to 23.2. backport-24.1.x Flags PRs that need to be backported to 24.1. labels Apr 11, 2024
@mgartner
Copy link
Collaborator Author

blathers backport 23.1

Copy link

blathers-crl bot commented Apr 11, 2024

Encountered an error creating backports. Some common things that can go wrong:

  1. The backport branch might have already existed.
  2. There was a merge conflict.
  3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.


error creating merge commit from 32d981b to blathers/backport-release-23.1-120384: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 23.1 failed. See errors above.


🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@mgartner
Copy link
Collaborator Author

blathers backport 23.2

Copy link

blathers-crl bot commented Apr 11, 2024

Encountered an error creating backports. Some common things that can go wrong:

  1. The backport branch might have already existed.
  2. There was a merge conflict.
  3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.


error creating merge commit from 32d981b to blathers/backport-release-23.2-120384: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 23.2 failed. See errors above.


🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@mgartner
Copy link
Collaborator Author

blather backport 24.1

@rytaft
Copy link
Collaborator

rytaft commented Apr 11, 2024

blathers backport 24.1

Copy link

blathers-crl bot commented Apr 11, 2024

Encountered an error creating backports. Some common things that can go wrong:

  1. The backport branch might have already existed.
  2. There was a merge conflict.
  3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.


error creating merge commit from b78cfaa to blathers/backport-release-24.1-120384: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 24.1 failed. See errors above.


🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@mgartner
Copy link
Collaborator Author

Lol, thanks for the assist. Looks like I'm doing these all manually though. 🙃

@rytaft
Copy link
Collaborator

rytaft commented Apr 11, 2024

Haha oh well...

@mgartner mgartner removed the backport-24.1.x Flags PRs that need to be backported to 24.1. label Apr 12, 2024
@mgartner
Copy link
Collaborator Author

This doesn't need to be backported to 24.1—it was merged before the release-24.1 branch was cut.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-23.1.x Flags PRs that need to be backported to 23.1 backport-23.2.x Flags PRs that need to be backported to 23.2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants