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

allow query options to appear in any order any number of times #283

Merged
merged 8 commits into from
Oct 18, 2023

Conversation

jycor
Copy link

@jycor jycor commented Oct 18, 2023

Allow statements like this to parse:

select distinct sql_calc_found_rows distinct * from t;

Fixes dolthub/dolt#6829
Companion PR: dolthub/go-mysql-server#2088

Copy link
Member

@zachmu zachmu left a comment

Choose a reason for hiding this comment

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

Is it an error in MySQL to include an option more than once? What about mutually exclusive options? Like what happens if you do ALL and DISTINCT in the same query in mysql?

Comments Comments
Distinct string
Hints string
QueryOpts []string
Copy link
Member

Choose a reason for hiding this comment

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

We don't need each option as a separate field in the Select struct, but a string slice is too free form. You probably want to have a QueryOpts struct to hold these things during list construction with a .Merge() method like we have with column type options

Copy link
Member

@zachmu zachmu left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the changes

StraightJoinHint = "straight_join "
DistinctStr = "distinct "
AllStr = "all "
StraightJoinHintStr = "straight_join "
Copy link
Member

Choose a reason for hiding this comment

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

Do you still need these consts?

Copy link
Author

Choose a reason for hiding this comment

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

They're used in the Format() method, and some other parts of sql.y still rely on DistinctStr

@jycor jycor merged commit 45c2d7d into main Oct 18, 2023
4 checks passed
@jycor jycor deleted the james/found branch October 18, 2023 21:13
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.

SQL_CALC_FOUND_ROWS doesn't work?
2 participants