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

Refactor aggregation AST structs #10347

Merged
merged 27 commits into from
Jun 13, 2022
Merged

Refactor aggregation AST structs #10347

merged 27 commits into from
Jun 13, 2022

Conversation

rsajwani
Copy link
Contributor

@rsajwani rsajwani commented May 20, 2022 •
edited
Loading

Description

This is a refactoring, introducing new structs for aggregation functions, instead of using sqlparser.FuncExpr for them. It should make the rest of the code a little easier to work with. This helps us represent entities in much cleaner way and reduces the number of checks we need to perform. Putting them inside a separate interface helps us in abstraction as well.

I have introduced structure for each aggregate function. Here is the list of all aggregates (https://dev.mysql.com/doc/refman/8.0/en/aggregate-functions.html). Apart from json_* every other aggregate function is covered.

There are some suggestions by fmt during the merge so I did that as part of this PR as well. chk out main_test.go and vault_server.go

TODO: While initial review Andres (@systay) gave a feedback to consolidate all these struct into single entity and that will make code much simpler. But I would like to treat that refactoring as a separate PR. Please let me know if anyone fees strongly against it and I can do this change in this PR itself.

Related Issue(s)

Checklist

  • "Backport me!" label has been added if this change should be backported
  • Tests were added or are not required
  • Documentation was added or is not required

Deployment Notes

rsajwani and others added 13 commits May 19, 2022 14:45
@rsajwani
SQLParser:Refactoring Add count struct
8dbb5f1 
Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>
@rsajwani
SQLParser:Refactoring Add countStar struct
934fa96 
Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>
@rsajwani
SQLParser:Refactoring Add avg struct
d96a6df 
Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>
@rsajwani
SQLParser:Refactoring Add max struct
f79aa19 
Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>
@rsajwani
SQLParser:Refactoring Add min struct
28def9a 
Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>
@rsajwani
SQLParser:Refactoring Add sum struct
3005af2 
Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>
@rsajwani
SQLParser:Refactoring Fixing Parser Aggr Function
0bbb68c 
Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>
@systay
fix: the return type of count was wrong
4cb77cb 
Signed-off-by: Andres Taylor <andres@planetscale.com>
@rsajwani
Refacotring code
baca564 
Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>
@rsajwani
More refactoring and unit test cases fix
e50f583 
Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>
@rsajwani
removing getarg from aggregate interface
67d9ad6 
Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>
@systay
Merge branch main into Issue#10113
f1d013b 
Signed-off-by: Andres Taylor <andres@planetscale.com>
@rsajwani
Merge branch 'Issue#10113' of github.com:planetscale/vitess into Issu…
9df4f67 
…e#10113

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>
@systay systay changed the title Issue#10113 Refactor aggregation AST structs Jun 1, 2022
rsajwani added 4 commits June 1, 2022 18:47
@rsajwani
Fixing bugs after merge
9a1e265 
Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>
@rsajwani
Optimizing code
c73de21 
Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>
@rsajwani
Fixing planBuilder test cases`
e448a03 
Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>
@rsajwani
Merge branch 'main' into Issue#10113
c6ffbf7 
Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>
@rsajwani rsajwani added Component: Query Serving Type: Enhancement Logical improvement (somewhere between a bug and feature) release notes (needs details) This PR needs to be listed in the release notes in a dedicated section (deprecation notice, etc...) labels Jun 5, 2022
@rsajwani
Adding more aggregate functions
d5e2b49 
Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>
systay
systay reviewed Jun 7, 2022
View reviewed changes
systay
systay reviewed Jun 7, 2022
View reviewed changes
systay
systay reviewed Jun 7, 2022
View reviewed changes
systay
systay reviewed Jun 7, 2022
View reviewed changes
@rsajwani rsajwani marked this pull request as ready for review June 7, 2022 23:19
@github-actions
Copy link
Contributor

github-actions bot commented Jun 7, 2022

Review Checklist

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

General

  • Ensure that the Pull Request has the correct release notes label. release notes none should only be used for PRs that are so trivial that they need not be included.
  • If a new flag is being introduced, review whether it is really needed. The flag names should be clear and intuitive (as far as possible), and the flag's help should be descriptive.

Bug fixes

  • There should be at least one unit or end-to-end test.
  • The Pull Request description should either include a link to an issue that describes the bug OR an actual description of the bug and how to reproduce, along with a description of the fix.

Non-trivial changes

  • There should be some code comments as to why things are implemented the way they are.

New/Existing features

  • Should be documented, either by modifying the existing documentation or creating new documentation.
  • New features should have a link to a feature request issue or an RFC that documents the use cases, corner cases and test cases.

Backward compatibility

  • Protobuf changes should be wire-compatible.
  • Changes to _vt tables and RPCs need to be backward compatible.
  • vtctl command output order should be stable and awk-able.

harshit-gangal
harshit-gangal approved these changes Jun 8, 2022
View reviewed changes
Copy link
Member

@harshit-gangal harshit-gangal left a comment

Choose a reason for hiding this comment

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

overall looks really good.
just few nits here and there.

go/vt/sqlparser/ast.go
Comment on lines +2645 to +2651
Name string
}

CountStar struct {
Name string
}

Copy link
Member

Choose a reason for hiding this comment

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

does name represents alias here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

does name represents alias here?

Name represent (in this case) either 'count' or 'Count' or 'COUNT' etc ... given what I made out of test cases we preserve case sensitivity of keywords in query. Hence this field preserved what has been keyed by user.

rsajwani added 4 commits June 8, 2022 21:29
@rsajwani
fixing feedback , code review
792331d 
Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>
@rsajwani
Merge branch 'main' into Issue#10113
a16e3a6 
Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>
@rsajwani
Fix regression which making changes in previous commit
d801e2e 
Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>
@rsajwani
Fixing vdiff tests
ab9d821 
Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>
rsajwani added 2 commits June 9, 2022 15:03
@rsajwani
Fixing replication test
cb7d5c7 
Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>
@rsajwani
Fixing vstreamer planbuilder test
46a485f 
Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>
systay
systay reviewed Jun 13, 2022
View reviewed changes
go/vt/sqlparser/ast.go
func (*VarSamp) iExpr() {}
func (*Variance) iExpr() {}

func (sum *Sum) GetArg() Expr { return sum.Arg }
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: this breaks the pattern this file had before. I think it's nice to keep all the iExpr() together. maybe we could move the GetArg, IsDistinct and AggrName to after the iExprs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will do it in next PR .. I have already have a clean up PR following this as discussed offline ..

@rsajwani
remove redundant code
b895d13 
Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>
@deepthi deepthi merged commit fc4c09e into vitessio:main Jun 13, 2022
@deepthi deepthi deleted the Issue#10113 branch June 13, 2022 20:01
@GuptaManan100 GuptaManan100 mentioned this pull request Jun 14, 2022
Merged
3 tasks
@dbussink dbussink mentioned this pull request Jul 25, 2022
Merged
3 tasks
@GuptaManan100 GuptaManan100 mentioned this pull request Aug 10, 2022
Closed
3 tasks
systay added a commit to planetscale/vitess that referenced this pull request Aug 19, 2022
@vmg @rsajwani
@systay @K-Kumar-01 @GuptaManan100 @frouioui @harshit-gangal @rohit-nayak-ps @dbussink
Cherry Pick all SQLParser changes (vitessio#904)
c07c82b 
* Refactor aggregation AST structs (vitessio#10347)

* SQLParser:Refactoring Add count struct

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

* SQLParser:Refactoring Add countStar struct

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

* SQLParser:Refactoring Add avg struct

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

* SQLParser:Refactoring Add max struct

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

* SQLParser:Refactoring Add min struct

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

* SQLParser:Refactoring Add sum struct

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

* SQLParser:Refactoring Fixing Parser Aggr Function

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

* fix: the return type of count was wrong

Signed-off-by: Andres Taylor <andres@planetscale.com>

* Refacotring code

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

* More refactoring and unit test cases fix

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

* removing getarg from aggregate interface

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

* Fixing bugs after merge

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

* Optimizing code

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

* Fixing planBuilder test cases`

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

* Adding more aggregate functions

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

* Fixing parser errors

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

* fixing feedback , code review

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

* Fix regression which making changes in previous commit

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

* Fixing vdiff tests

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

* Fixing replication test

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

* Fixing vstreamer planbuilder test

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

* remove redundant code

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

Co-authored-by: Andres Taylor <andres@planetscale.com>
Signed-off-by: Vicent Marti <vmg@strn.cat>

* Add parsing support for performance schema functions (vitessio#10478)

* feat: add parsing support for performance schema functions

Signed-off-by: Kushal Kumar <kushalkumargupta4@gmail.com>

* fix: update sql.go by making parser

Signed-off-by: Kushal Kumar <kushalkumargupta4@gmail.com>
Signed-off-by: Vicent Marti <vmg@strn.cat>

* Reduce shift-reduce conflicts (vitessio#10500)

* feat: reduce shift-reduce conflicts by using the precedence symbol FUNCTION_CALL_NON_KEYWORD

Signed-off-by: Manan Gupta <manan@planetscale.com>

* feat: fix static check workflow to setup go in cases of parser changes too

Signed-off-by: Manan Gupta <manan@planetscale.com>
Signed-off-by: Vicent Marti <vmg@strn.cat>

* Cleanup:  Remove 'Name' field from aggregate structure (vitessio#10507)

* Code clean up

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

* Fixing unit test case failures

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

* Fixing replicator planner unit tests

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

* Adding unit test cases of canonical output

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>

* Fix unit test

Signed-off-by: Rameez Sajwani <rameezwazirali@hotmail.com>
Signed-off-by: Vicent Marti <vmg@strn.cat>

* User defined and sys variables (vitessio#10547)

* feat: add user defined variables and sys variables as separate structs and use user-defined variable in Execute statement

Signed-off-by: Manan Gupta <manan@planetscale.com>

* feat: remove parsing of at_id and at_at_id from places that shouldn't parse it

Signed-off-by: Manan Gupta <manan@planetscale.com>
Signed-off-by: Andres Taylor <andres@planetscale.com>

* feat: use ci_identifier in more places

Signed-off-by: Andres Taylor <andres@planetscale.com>

* refactor: clean up the JSON AST structs a little

Signed-off-by: Andres Taylor <andres@planetscale.com>

* refactor: use remove unnesseccary scope

Signed-off-by: Andres Taylor <andres@planetscale.com>

* Created new Variable node to divide the work of ColName

Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>

* refactor fix sqlparser: SET ast and parsing

Signed-off-by: Andres Taylor <andres@planetscale.com>

* refactor: remove unused ast-structs

Signed-off-by: Andres Taylor <andres@planetscale.com>

* refactor: clean up how variables are parsed and formatted

Signed-off-by: Andres Taylor <andres@planetscale.com>

* fix: make the new AST work in the planbuilder

Signed-off-by: Andres Taylor <andres@planetscale.com>

* refactor: rename ColIdent and TableIdent

Signed-off-by: Andres Taylor <andres@planetscale.com>

* fix: set name should be parsed with SessionScope

Signed-off-by: Andres Taylor <andres@planetscale.com>

* chore: clean and document stuff

Signed-off-by: Andres Taylor <andres@planetscale.com>

* test: update test expectation

Signed-off-by: Andres Taylor <andres@planetscale.com>

* fix: default scope for transaction isolation should not be session

Signed-off-by: Andres Taylor <andres@planetscale.com>

* feat: handle local as a synonym for session for variable scopes

Signed-off-by: Andres Taylor <andres@planetscale.com>

* comments: clean up code comments

Signed-off-by: Andres Taylor <andres@planetscale.com>

Co-authored-by: Manan Gupta <manan@planetscale.com>
Co-authored-by: Florent Poinsard <florent.poinsard@outlook.fr>
Signed-off-by: Vicent Marti <vmg@strn.cat>

* Revert "[Backport 14.0] enable schema tracking by default  (vitessio#10595)"

This reverts commit 8ebe1cc.

Signed-off-by: Vicent Marti <vmg@strn.cat>

* enable schema tracking by default (vitessio#10455)

* feat: enable schema tracking by default

Signed-off-by: Harshit Gangal <harshit@planetscale.com>

* test: fix test setup

Signed-off-by: Harshit Gangal <harshit@planetscale.com>

* test: fix vschema test setup

Signed-off-by: Harshit Gangal <harshit@planetscale.com>

* test: turn off schema tracking on the tablet

Signed-off-by: Andres Taylor <andres@planetscale.com>

* test: fix test assertion

Signed-off-by: Andres Taylor <andres@planetscale.com>

* Change read query for checks that test to which keyspace a table is routed to. This uses the /queryz vttablet endpoint where the query gets expanded if schema tracking is enabled, hence failing an exact query check

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

* test: only use gen4 planner

Signed-off-by: Harshit Gangal <harshit@planetscale.com>

* fix: column list population in insert with auto-inc column

Signed-off-by: Harshit Gangal <harshit@planetscale.com>

* fix: change parser to keep empty column list as provided by user

Signed-off-by: Harshit Gangal <harshit@planetscale.com>

* test: fix test expectation

Signed-off-by: Harshit Gangal <harshit@planetscale.com>

Co-authored-by: Andres Taylor <andres@planetscale.com>
Co-authored-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Vicent Marti <vmg@strn.cat>

* Add parsing support for GTID functions (vitessio#10579)

* feat: add parsing support for GTID functions

Signed-off-by: Kushal Kumar <kushalkumargupta4@gmail.com>

* test: add planner and end to end tests for a few gtid functions

Signed-off-by: Manan Gupta <manan@planetscale.com>

Co-authored-by: Manan Gupta <manan@planetscale.com>
Signed-off-by: Vicent Marti <vmg@strn.cat>

* Parameterize BIT types and fixes in HEX types (vitessio#10689)

* feat: add parsing for bitnums

Signed-off-by: Manan Gupta <manan@planetscale.com>

* test: add invalid cases to tests

Signed-off-by: Manan Gupta <manan@planetscale.com>

* feat: normalize bitnums to bit vals too

Signed-off-by: Manan Gupta <manan@planetscale.com>

* feat: add normalization for bit literals

Signed-off-by: Manan Gupta <manan@planetscale.com>

* feat: parameterize binary value to hex

Signed-off-by: Harshit Gangal <harshit@planetscale.com>

* test: added e2e test in vtgate and vttablet

Signed-off-by: Harshit Gangal <harshit@planetscale.com>

* fix: fix the type conversion from hexnum and hexval to binary

Signed-off-by: Harshit Gangal <harshit@planetscale.com>

* test: refactor test

Signed-off-by: Harshit Gangal <harshit@planetscale.com>

* fix: return varbinary and flaghex for hexval and hexnum in eval engine typeOf

Signed-off-by: Harshit Gangal <harshit@planetscale.com>

* test: separate expectation for mysql and vitess result

Signed-off-by: Harshit Gangal <harshit@planetscale.com>

* test: fix test expectation

Signed-off-by: Harshit Gangal <harshit@planetscale.com>

* feat: added bitnum bind variable and test

Signed-off-by: Harshit Gangal <harshit@planetscale.com>

* proto: vtadmin side update

Signed-off-by: Harshit Gangal <harshit@planetscale.com>

* test: fixed test and added more bitnum bind var test

Signed-off-by: Harshit Gangal <harshit@planetscale.com>

* added license header

Signed-off-by: Harshit Gangal <harshit@planetscale.com>

Co-authored-by: Manan Gupta <manan@planetscale.com>
Signed-off-by: Vicent Marti <vmg@strn.cat>

* Mark aggregate functions callable (vitessio#10805)

These should also implement the iCallable interface as they are also
callable as functions.

Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>
Signed-off-by: Vicent Marti <vmg@strn.cat>

* cherry-pick: fix bad merge

Signed-off-by: Vicent Marti <vmg@strn.cat>

Co-authored-by: rsajwani <rameezwazirali@hotmail.com>
Co-authored-by: Andres Taylor <andres@planetscale.com>
Co-authored-by: Kushal Kumar <59891164+K-Kumar-01@users.noreply.github.com>
Co-authored-by: Manan Gupta <35839558+GuptaManan100@users.noreply.github.com>
Co-authored-by: Manan Gupta <manan@planetscale.com>
Co-authored-by: Florent Poinsard <florent.poinsard@outlook.fr>
Co-authored-by: Harshit Gangal <harshit@planetscale.com>
Co-authored-by: Rohit Nayak <rohit@planetscale.com>
Co-authored-by: Dirkjan Bussink <d.bussink@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@systay systay systay left review comments

@harshit-gangal harshit-gangal harshit-gangal approved these changes

@shlomi-noach shlomi-noach Awaiting requested review from shlomi-noach shlomi-noach is a code owner

@frouioui frouioui Awaiting requested review from frouioui frouioui is a code owner

@GuptaManan100 GuptaManan100 Awaiting requested review from GuptaManan100 GuptaManan100 is a code owner

@deepthi deepthi Awaiting requested review from deepthi deepthi is a code owner

@rohit-nayak-ps rohit-nayak-ps Awaiting requested review from rohit-nayak-ps rohit-nayak-ps is a code owner

@mattlord mattlord Awaiting requested review from mattlord mattlord is a code owner

Assignees

@rsajwani rsajwani

Labels
Component: Query Serving release notes (needs details) This PR needs to be listed in the release notes in a dedicated section (deprecation notice, etc...) Type: Enhancement Logical improvement (somewhere between a bug and feature)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rsajwani @systay @harshit-gangal @deepthi