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

Implement parser support for all select modifiers #16396

Merged
merged 6 commits into from
Jul 19, 2024

Conversation

Utkar5hM
Copy link
Contributor

@Utkar5hM Utkar5hM commented Jul 16, 2024

Description

Implemented support for the following select modifiers in the sqlparser:

[HIGH_PRIORITY]
[SQL_SMALL_RESULT]
[SQL_BIG_RESULT]
[SQL_BUFFER_RESULT]

Added simple test cases for each of the modifiers under go/vt/sqlparser/parse_test.go similarly to that of sql_calc_found_rows.

I'll update the semantics check for the following modifiers and sql_calc_found_rows in a separate PR as discussed on slack.

Related Issue(s)

Fixes #15318

Checklist

  • "Backport to:" labels have been added if this change should be back-ported to release branches
  • If this change is to be back-ported to previous releases, a justification is included in the PR description
  • Tests were added or are not required
  • Did the new or modified tests pass consistently locally and on CI?
  • Documentation was added or is not required

Deployment Notes

Signed-off-by: Utkarsh Mahajan <utkarshrm568@gmail.com>
Copy link
Contributor

vitess-bot bot commented Jul 16, 2024

Review Checklist

Hello reviewers! 👋 Please follow this checklist when reviewing this Pull Request.

General

  • Ensure that the Pull Request has a descriptive title.
  • Ensure there is a link to an issue (except for internal cleanup and flaky test fixes), new features should have an RFC that documents use cases and test cases.

Tests

  • Bug fixes should have at least one unit or end-to-end test, enhancement and new features should have a sufficient number of tests.

Documentation

  • Apply the release notes (needs details) label if users need to know about this change.
  • New features should be documented.
  • There should be some code comments as to why things are implemented the way they are.
  • There should be a comment at the top of each new or modified test to explain what the test does.

New flags

  • Is this flag really necessary?
  • Flag names must be clear and intuitive, use dashes (-), and have a clear help text.

If a workflow is added or modified:

  • Each item in Jobs should be named in order to mark it as required.
  • If the workflow needs to be marked as required, the maintainer team must be notified.

Backward compatibility

  • Protobuf changes should be wire-compatible.
  • Changes to _vt tables and RPCs need to be backward compatible.
  • RPC changes should be compatible with vitess-operator
  • If a flag is removed, then it should also be removed from vitess-operator and arewefastyet, if used there.
  • vtctl command output order should be stable and awk-able.

@vitess-bot vitess-bot bot added NeedsBackportReason If backport labels have been applied to a PR, a justification is required NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work NeedsIssue A linked issue is missing for this Pull Request NeedsWebsiteDocsUpdate What it says labels Jul 16, 2024
@github-actions github-actions bot added this to the v21.0.0 milestone Jul 16, 2024
@systay systay added Type: Enhancement Logical improvement (somewhere between a bug and feature) Component: Query Serving and removed NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work NeedsWebsiteDocsUpdate What it says NeedsIssue A linked issue is missing for this Pull Request NeedsBackportReason If backport labels have been applied to a PR, a justification is required labels Jul 16, 2024
@dbussink
Copy link
Contributor

@Utkar5hM You'll also need to update the other test fixtures. If you run them, it should tell you how you can copy over the new expectations.

…he select modifiers

Signed-off-by: Utkarsh Mahajan <utkarshrm568@gmail.com>
@Utkar5hM
Copy link
Contributor Author

Utkar5hM commented Jul 16, 2024

  • updated the test cases to support the select modifiers.

  • I've added SQL_BUFFER_RESULT under non reserved keywords list in sql.y.

  • made a change to the func testFile() to make the newly generated select_case.txt be easily copyable in case it's the correct one. Previously, a error generating statement would still produce a error output text even if the error is null and there is an actual output which would require a lot of manual work changing ERROR to OUTPUT and regenerating select_case.txt.

  • However the particular test case at https://github.com/vitessio/vitess/blob/main/go/vt/sqlparser/testdata/select_cases.txt#L19706 which errored due to sql_big_result not being a modifier now passes.

INPUT
select sql_big_result distinct t1.a from t1,t2 order by t2.a;
END
ERROR
syntax error at position 31 near 'distinct'
END

now produces

OUTPUT
select distinct sql_big_result t1.a from t1, t2 order by t2.a asc

which I think should fail as it is still an incorrect query as can be seen below or is this fine here? :
image

@@ -8497,6 +8513,7 @@ non_reserved_keyword:
| SNAPSHOT
| SOME %prec ANY_SOME
| SQL
| SQL_BUFFER_RESULT
Copy link
Member

@GuptaManan100 GuptaManan100 Jul 17, 2024

Choose a reason for hiding this comment

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

HIGH_PRIORITY, SQL_SMALL_RESULT and SQL_BIG_RESULT need to be added to the reserved_keyword list.

Copy link

codecov bot commented Jul 17, 2024

Codecov Report

Attention: Patch coverage is 80.95238% with 4 lines in your changes missing coverage. Please review.

Project coverage is 68.68%. Comparing base (3cb61cd) to head (b92da94).
Report is 15 commits behind head on main.

Files Patch % Lines
go/vt/sqlparser/ast_format.go 50.00% 4 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main   #16396   +/-   ##
=======================================
  Coverage   68.68%   68.68%           
=======================================
  Files        1548     1548           
  Lines      199084   199116   +32     
=======================================
+ Hits       136736   136766   +30     
- Misses      62348    62350    +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Utkarsh Mahajan <utkarshrm568@gmail.com>
@Utkar5hM
Copy link
Contributor Author

I have updated the above.

Addition of SQL_BUFFER_RESULT seems to have caused 3 shift/reduce conflicts making static Code check workflow to fail.

1732: shift/reduce conflict (shift 1738(0), red'n 901(0)) on SQL_BUFFER_RESULT
state 1732
	select_options_opt:  select_options.    (901)
	select_options:  select_options.select_option 

	DISTINCT  shift 1734
	ALL  shift 1742
	DISTINCTROW  shift 1735
	SQL_NO_CACHE  shift 2367
	SQL_CACHE  shift 2368
	SQL_CALC_FOUND_ROWS  shift 1741
	SQL_SMALL_RESULT  shift 1739
	SQL_BIG_RESULT  shift 1740
	SQL_BUFFER_RESULT  shift 1738
	HIGH_PRIORITY  shift 1736
	STRAIGHT_JOIN  shift 1737
	.  reduce 901 (src line 4801)

	select_option  goto 2372
1727: shift/reduce conflict (shift 1738(0), red'n 900(0)) on SQL_BUFFER_RESULT
state 1727
	query_primary:  SELECT comment_opt.select_options_opt select_expression_list into_clause from_opt where_expression_opt group_by_opt having_opt named_windows_list_opt 
	query_primary:  SELECT comment_opt.select_options_opt select_expression_list from_opt where_expression_opt group_by_opt having_opt named_windows_list_opt 
	select_options_opt: .    (900)

	DISTINCT  shift 1734
	ALL  shift 1742
	DISTINCTROW  shift 1735
	SQL_NO_CACHE  shift 2367
	SQL_CACHE  shift 2368
	SQL_CALC_FOUND_ROWS  shift 1741
	SQL_SMALL_RESULT  shift 1739
	SQL_BIG_RESULT  shift 1740
	SQL_BUFFER_RESULT  shift 1738
	HIGH_PRIORITY  shift 1736
	STRAIGHT_JOIN  shift 1737
	.  reduce 900 (src line 4797)

	select_options  goto 1732
	select_options_opt  goto 1729
	select_option  goto 1733
1037: shift/reduce conflict (shift 1738(0), red'n 900(0)) on SQL_BUFFER_RESULT
state 1037
	query_expression:  SELECT comment_opt.cache_opt NEXT num_val for_from table_name 
	query_primary:  SELECT comment_opt.select_options_opt select_expression_list into_clause from_opt where_expression_opt group_by_opt having_opt named_windows_list_opt 
	query_primary:  SELECT comment_opt.select_options_opt select_expression_list from_opt where_expression_opt group_by_opt having_opt named_windows_list_opt 
	cache_opt: .    (887)
	select_options_opt: .    (900)

	DISTINCT  shift 1734
	ALL  shift 1742
	DISTINCTROW  shift 1735
	NEXT  reduce 887 (src line 4732)
	SQL_NO_CACHE  shift 1730
	SQL_CACHE  shift 1731
	SQL_CALC_FOUND_ROWS  shift 1741
	SQL_SMALL_RESULT  shift 1739
	SQL_BIG_RESULT  shift 1740
	SQL_BUFFER_RESULT  shift 1738
	HIGH_PRIORITY  shift 1736
	STRAIGHT_JOIN  shift 1737
	.  reduce 900 (src line 4797)

	cache_opt  goto 1728
	select_options  goto 1732
	select_options_opt  goto 1729
	select_option  goto 1733

I'm pretty new to yacc and been having a look into how to eliminate this conflicts. I might take some time understanding it.

Signed-off-by: Manan Gupta <manan@planetscale.com>
@GuptaManan100
Copy link
Member

I have fixed the shift-reduce conflicts for you.

Signed-off-by: Utkarsh Mahajan <utkarshrm568@gmail.com>
systay

This comment was marked as resolved.

Copy link
Collaborator

@systay systay left a comment

Choose a reason for hiding this comment

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

I was reading it wrong. This LGTM!

Signed-off-by: Manan Gupta <manan@planetscale.com>
@systay systay merged commit efd8292 into vitessio:main Jul 19, 2024
127 checks passed
frouioui pushed a commit to frouioui/vitess that referenced this pull request Jul 19, 2024
Signed-off-by: Utkarsh Mahajan <utkarshrm568@gmail.com>
Signed-off-by: Manan Gupta <manan@planetscale.com>
Co-authored-by: Manan Gupta <manan@planetscale.com>
Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Query Serving Type: Enhancement Logical improvement (somewhere between a bug and feature)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support all SELECT modifiers
4 participants