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.

Sign up for GitHub

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

Jump to bottom

parser: support empty target list in SELECT #116835

Merged
merged 1 commit into from
Jan 5, 2024
Merged

parser: support empty target list in SELECT #116835

merged 1 commit into from
Jan 5, 2024

Conversation

yuzefovich
Copy link
Member

@yuzefovich yuzefovich commented Dec 20, 2023 •
edited
Loading

This commit adds support for parsing empty target list for SELECT stmts (i.e. queries of the form SELECT;, SELECT ALL FROM t;, SELECT FROM t; where the projection column list is omitted). Apparently, some tools like hasura use it to get the row count.

This is achieved by making target_list optional. However, the caveat is that the new parsing rule runs into a shift/reduce conflict with SELECT error rule (since now both options can match empty string after SELECT AFAICT). I tried adjusting the precedence of the newly adjusted rule to resolve this conflict but didn't succeed (Raphael tried doing the same with the same result).

Fixes: #83255.

Release note (sql change): CockroachDB now supports parsing queries like SELECT FROM t that only produce the row count and do not output any columns.

@yuzefovich yuzefovich requested review from rafiss, a team and michae2 and removed request for a team December 20, 2023 00:19
@yuzefovich yuzefovich requested review from a team as code owners December 20, 2023 00:19
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@yuzefovich yuzefovich mentioned this pull request Dec 20, 2023
Closed
@yuzefovich yuzefovich force-pushed the empty-select branch 2 times, most recently from a4a47eb to dcd209f Compare December 20, 2023 01:00
@michae2
Copy link
Collaborator

michae2 commented Dec 20, 2023

Nice! I like this.

(I vote that you do VALUES (); next!)

Any suggestions on how to preserve the contextual help rule would be appreciated.

I don't think there is a way to preserve this exact contextual help rule, since SELECT; is no longer a syntax error. But maybe you could add more contextual help rules for other patterns like:

  • SELECT opt_all_clause target_list ',' error
  • WHERE error
  • SELECT distinct_clause error
  • SELECT distinct_on_clause error

@yuzefovich
Copy link
Member Author

Interestingly, Raphael arrived at the same solution to this issue and also didn't see a way to not remove the contextual help.

I'll play around with some rules to preserve the contextual help.

@yuzefovich
Copy link
Member Author

Added these suggestions which reverted 2 of the tests to the previous state. However, I'm not sure whether these new rules are actually worth it - like the grammar now looks even dirtier, and this help in a few cases might not be that helpful. These new rules are also quite arbitrary - we could probably come up with a dozen of others, so why stop here?

So I'm leaning towards not adding these new error rules, and the question is whether we're ok removing the contextual error help in order to support this syntax. It seems worth it to me. Thoughts?

rafiss
rafiss approved these changes Dec 28, 2023
View reviewed changes
Copy link
Collaborator

@rafiss rafiss left a comment

Choose a reason for hiding this comment

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

lgtm! i think losing the help output for SELECT (with no other tokens) is fine

michae2
michae2 reviewed Jan 2, 2024
View reviewed changes
Copy link
Collaborator

@michae2 michae2 left a comment

Choose a reason for hiding this comment

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

Will the \h SELECT command in the cli still work?

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained

@yuzefovich
parser: support empty target list in SELECT
da147cf 
This commit adds support for parsing empty target list for SELECT stmts
(i.e. queries of the form `SELECT;`, `SELECT ALL FROM t;`, `SELECT FROM t;`
where the projection column list is omitted). Apparently, some tools
like hasura use it to get the row count.

This is achieved by making `target_list` optional. However, the caveat
is that the new parsing rule runs into a shift/reduce conflict with
`SELECT error` rule (since now both options can match empty string after
`SELECT` AFAICT). I tried adjusting the precedence of the newly adjusted
rule to resolve this conflict but didn't succeed (Raphael tried doing
the same with the same result).

Release note (sql change): CockroachDB now supports parsing queries like
`SELECT FROM t` that only produce the row count and do not output any
columns.
yuzefovich
yuzefovich commented Jan 4, 2024
View reviewed changes
Copy link
Member Author

@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.

\h SELECT still works, yes. @michae2 are you ok with this change without any extra rules for partial error support too?

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained

michae2
michae2 approved these changes Jan 4, 2024
View reviewed changes
Copy link
Collaborator

@michae2 michae2 left a comment

Choose a reason for hiding this comment

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

LGTM!

@yuzefovich
Copy link
Member Author

TFTRs!

bors r+

@craig
Copy link
Contributor

craig bot commented Jan 4, 2024

This PR was included in a batch that timed out, it will be automatically retried

@yuzefovich
Copy link
Member Author

bors r+

@craig
Copy link
Contributor

craig bot commented Jan 4, 2024

Already running a review

@craig
Copy link
Contributor

craig bot commented Jan 5, 2024

This PR was included in a batch that timed out, it will be automatically retried

@craig
Copy link
Contributor

craig bot commented Jan 5, 2024

Build succeeded:

@craig craig bot merged commit 1d16a91 into cockroachdb:master Jan 5, 2024
5 checks passed
@yuzefovich yuzefovich deleted the empty-select branch January 6, 2024 02:08
rafiss added a commit to rafiss/cockroach that referenced this pull request Jan 8, 2024
@rafiss
roachtest: mark asyncpg test as passing
7033716 
This started passing after
cockroachdb#116835 was merged.

Release note: None
@rafiss rafiss mentioned this pull request Jan 8, 2024
Merged
craig bot pushed a commit that referenced this pull request Jan 11, 2024
@rafiss
Merge #117516 #117580
695f7ef 
117516: roachtest: mark asyncpg test as passing r=rafiss a=rafiss

This started passing after
#116835 was merged.

fixes #117379
Release note: None

117580: workload: a few fixes for schema change watchdog r=rafiss a=rafiss

The log message it created on failure did not produce readable output. Also, the watchdog could get confused if a query completed and the session ended up in an idle state.

fixes #117533
Release note: None

Co-authored-by: Rafi Shamim <[email protected]>
@soupi soupi mentioned this pull request Mar 4, 2024
Merged
andrewbaptist pushed a commit to andrewbaptist/cockroach that referenced this pull request Mar 6, 2024
@rafiss @andrewbaptist
roachtest: mark asyncpg test as passing
b1f9ebc 
This started passing after
cockroachdb#116835 was merged.

Release note: None
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rafiss rafiss rafiss approved these changes

@michae2 michae2 michae2 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

sql: support for SELECT with no projections
4 participants
@yuzefovich @cockroach-teamcity @michae2 @rafiss