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

User defined and sys variables #10547

Merged
merged 18 commits into from
Jun 20, 2022
Merged

User defined and sys variables #10547

merged 18 commits into from
Jun 20, 2022

Conversation

systay
Copy link
Collaborator

@systay systay commented Jun 20, 2022 •
edited by deepthi
Loading

Description

This PR fixes a long standing technical debt where we would encode column expressions and variables using the same structs. This led to at least one bug with the V3 planner where it would try to find which table a variable came from, which doesn't make much sense.

I also took the opportunity when we are changing this basic building blocks of the AST, to rename a couple of structs that are used in lots of places in the code.

These are ColIdent and TableIdent. They have been renamed to IdentifierCI and IdentifierCS (case sensitive/insensitive).

After this change, ColName is used for column expressions, Variable for user defined and system variables. The old ColIdent, now known as IdentifierCS no longer contains information about the number of @ symbols.

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

GuptaManan100 and others added 18 commits June 15, 2022 17:35
@GuptaManan100 @systay
feat: add user defined variables and sys variables as separate struct…
468e06e 
…s and use user-defined variable in Execute statement

Signed-off-by: Manan Gupta <[email protected]>
@GuptaManan100 @systay
feat: remove parsing of at_id and at_at_id from places that shouldn't…
9bd16be 
… parse it

Signed-off-by: Manan Gupta <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
@systay
feat: use ci_identifier in more places
7d4099e 
Signed-off-by: Andres Taylor <[email protected]>
@systay
refactor: clean up the JSON AST structs a little
7d29835 
Signed-off-by: Andres Taylor <[email protected]>
@systay
refactor: use remove unnesseccary scope
f29ef99 
Signed-off-by: Andres Taylor <[email protected]>
@frouioui @systay
Created new Variable node to divide the work of ColName
2cc61ca 
Signed-off-by: Florent Poinsard <[email protected]>
@systay
refactor fix sqlparser: SET ast and parsing
a43f5a5 
Signed-off-by: Andres Taylor <[email protected]>
@systay
refactor: remove unused ast-structs
26afd25 
Signed-off-by: Andres Taylor <[email protected]>
@systay
refactor: clean up how variables are parsed and formatted
4d96779 
Signed-off-by: Andres Taylor <[email protected]>
@systay
fix: make the new AST work in the planbuilder
6fad74b 
Signed-off-by: Andres Taylor <[email protected]>
@systay
refactor: rename ColIdent and TableIdent
f2af53c 
Signed-off-by: Andres Taylor <[email protected]>
@systay
fix: set name should be parsed with SessionScope
e78f146 
Signed-off-by: Andres Taylor <[email protected]>
@systay
chore: clean and document stuff
72e7dd7 
Signed-off-by: Andres Taylor <[email protected]>
@systay
test: update test expectation
c5ab11a 
Signed-off-by: Andres Taylor <[email protected]>
@systay
fix: default scope for transaction isolation should not be session
3f51c4f 
Signed-off-by: Andres Taylor <[email protected]>
@systay
feat: handle local as a synonym for session for variable scopes
d8a6f8a 
Signed-off-by: Andres Taylor <[email protected]>
@systay
Merge branch main into user-defined-and-sys-variables
1963c08 
Signed-off-by: Andres Taylor <[email protected]>
@systay
comments: clean up code comments
19b94a0 
Signed-off-by: Andres Taylor <[email protected]>
@systay systay mentioned this pull request Jun 20, 2022
Closed
3 tasks
@github-actions
Copy link
Contributor

Review Checklist

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

General

  • Ensure that the Pull Request has a descriptive title.
  • If this is a change that users need to know about, please apply the release notes (needs details) label so that merging is blocked unless the summary release notes document is 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.
  • If a workflow is added or modified, each items in Jobs should be named in order to mark it as required. If the workflow should be required, the GitHub Admin should be notified.

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.

GuptaManan100
GuptaManan100 reviewed Jun 20, 2022
View reviewed changes
Copy link
Member

@GuptaManan100 GuptaManan100 left a comment

Choose a reason for hiding this comment

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

This is the only unaddressed issue left. Other than this, LGTM!

go/vt/sqlparser/sql.y
@@ -6942,26 +6954,35 @@ set_list:
}

set_expression:
reserved_sql_id '=' ON
set_variable '=' ON
Copy link
Member

Choose a reason for hiding this comment

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

set_exprsesion, apparently also used to take in non-reserved and reserved keywords. It does not now. Is this change intentional?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, this is the way that the old code was doing it. I've been trying to find any valid SET queries that pass on mysql and fail on vitess because of this rule and I have not been able to find any. Since this is simpler, I'm going to leave it as is.

@systay systay merged commit 3f25ae8 into vitessio:main Jun 20, 2022
@systay systay deleted the user-defined-and-sys-variables branch June 20, 2022 11:52
@deepthi
Copy link
Member

deepthi commented Jun 24, 2022

Nice work!

@harshit-gangal harshit-gangal restored the user-defined-and-sys-variables branch June 28, 2022 06:50
@systay systay mentioned this pull request Jun 28, 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 <[email protected]>

* SQLParser:Refactoring Add countStar struct

Signed-off-by: Rameez Sajwani <[email protected]>

* SQLParser:Refactoring Add avg struct

Signed-off-by: Rameez Sajwani <[email protected]>

* SQLParser:Refactoring Add max struct

Signed-off-by: Rameez Sajwani <[email protected]>

* SQLParser:Refactoring Add min struct

Signed-off-by: Rameez Sajwani <[email protected]>

* SQLParser:Refactoring Add sum struct

Signed-off-by: Rameez Sajwani <[email protected]>

* SQLParser:Refactoring Fixing Parser Aggr Function

Signed-off-by: Rameez Sajwani <[email protected]>

* fix: the return type of count was wrong

Signed-off-by: Andres Taylor <[email protected]>

* Refacotring code

Signed-off-by: Rameez Sajwani <[email protected]>

* More refactoring and unit test cases fix

Signed-off-by: Rameez Sajwani <[email protected]>

* removing getarg from aggregate interface

Signed-off-by: Rameez Sajwani <[email protected]>

* Fixing bugs after merge

Signed-off-by: Rameez Sajwani <[email protected]>

* Optimizing code

Signed-off-by: Rameez Sajwani <[email protected]>

* Fixing planBuilder test cases`

Signed-off-by: Rameez Sajwani <[email protected]>

* Adding more aggregate functions

Signed-off-by: Rameez Sajwani <[email protected]>

* Fixing parser errors

Signed-off-by: Rameez Sajwani <[email protected]>

* fixing feedback , code review

Signed-off-by: Rameez Sajwani <[email protected]>

* Fix regression which making changes in previous commit

Signed-off-by: Rameez Sajwani <[email protected]>

* Fixing vdiff tests

Signed-off-by: Rameez Sajwani <[email protected]>

* Fixing replication test

Signed-off-by: Rameez Sajwani <[email protected]>

* Fixing vstreamer planbuilder test

Signed-off-by: Rameez Sajwani <[email protected]>

* remove redundant code

Signed-off-by: Rameez Sajwani <[email protected]>

Co-authored-by: Andres Taylor <[email protected]>
Signed-off-by: Vicent Marti <[email protected]>

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

* feat: add parsing support for performance schema functions

Signed-off-by: Kushal Kumar <[email protected]>

* fix: update sql.go by making parser

Signed-off-by: Kushal Kumar <[email protected]>
Signed-off-by: Vicent Marti <[email protected]>

* 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 <[email protected]>

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

Signed-off-by: Manan Gupta <[email protected]>
Signed-off-by: Vicent Marti <[email protected]>

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

* Code clean up

Signed-off-by: Rameez Sajwani <[email protected]>

* Fixing unit test case failures

Signed-off-by: Rameez Sajwani <[email protected]>

* Fixing replicator planner unit tests

Signed-off-by: Rameez Sajwani <[email protected]>

* Adding unit test cases of canonical output

Signed-off-by: Rameez Sajwani <[email protected]>

* Fix unit test

Signed-off-by: Rameez Sajwani <[email protected]>
Signed-off-by: Vicent Marti <[email protected]>

* 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 <[email protected]>

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

Signed-off-by: Manan Gupta <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>

* feat: use ci_identifier in more places

Signed-off-by: Andres Taylor <[email protected]>

* refactor: clean up the JSON AST structs a little

Signed-off-by: Andres Taylor <[email protected]>

* refactor: use remove unnesseccary scope

Signed-off-by: Andres Taylor <[email protected]>

* Created new Variable node to divide the work of ColName

Signed-off-by: Florent Poinsard <[email protected]>

* refactor fix sqlparser: SET ast and parsing

Signed-off-by: Andres Taylor <[email protected]>

* refactor: remove unused ast-structs

Signed-off-by: Andres Taylor <[email protected]>

* refactor: clean up how variables are parsed and formatted

Signed-off-by: Andres Taylor <[email protected]>

* fix: make the new AST work in the planbuilder

Signed-off-by: Andres Taylor <[email protected]>

* refactor: rename ColIdent and TableIdent

Signed-off-by: Andres Taylor <[email protected]>

* fix: set name should be parsed with SessionScope

Signed-off-by: Andres Taylor <[email protected]>

* chore: clean and document stuff

Signed-off-by: Andres Taylor <[email protected]>

* test: update test expectation

Signed-off-by: Andres Taylor <[email protected]>

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

Signed-off-by: Andres Taylor <[email protected]>

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

Signed-off-by: Andres Taylor <[email protected]>

* comments: clean up code comments

Signed-off-by: Andres Taylor <[email protected]>

Co-authored-by: Manan Gupta <[email protected]>
Co-authored-by: Florent Poinsard <[email protected]>
Signed-off-by: Vicent Marti <[email protected]>

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

This reverts commit 8ebe1cc.

Signed-off-by: Vicent Marti <[email protected]>

* enable schema tracking by default (vitessio#10455)

* feat: enable schema tracking by default

Signed-off-by: Harshit Gangal <[email protected]>

* test: fix test setup

Signed-off-by: Harshit Gangal <[email protected]>

* test: fix vschema test setup

Signed-off-by: Harshit Gangal <[email protected]>

* test: turn off schema tracking on the tablet

Signed-off-by: Andres Taylor <[email protected]>

* test: fix test assertion

Signed-off-by: Andres Taylor <[email protected]>

* 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 <[email protected]>

* test: only use gen4 planner

Signed-off-by: Harshit Gangal <[email protected]>

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

Signed-off-by: Harshit Gangal <[email protected]>

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

Signed-off-by: Harshit Gangal <[email protected]>

* test: fix test expectation

Signed-off-by: Harshit Gangal <[email protected]>

Co-authored-by: Andres Taylor <[email protected]>
Co-authored-by: Rohit Nayak <[email protected]>
Signed-off-by: Vicent Marti <[email protected]>

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

* feat: add parsing support for GTID functions

Signed-off-by: Kushal Kumar <[email protected]>

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

Signed-off-by: Manan Gupta <[email protected]>

Co-authored-by: Manan Gupta <[email protected]>
Signed-off-by: Vicent Marti <[email protected]>

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

* feat: add parsing for bitnums

Signed-off-by: Manan Gupta <[email protected]>

* test: add invalid cases to tests

Signed-off-by: Manan Gupta <[email protected]>

* feat: normalize bitnums to bit vals too

Signed-off-by: Manan Gupta <[email protected]>

* feat: add normalization for bit literals

Signed-off-by: Manan Gupta <[email protected]>

* feat: parameterize binary value to hex

Signed-off-by: Harshit Gangal <[email protected]>

* test: added e2e test in vtgate and vttablet

Signed-off-by: Harshit Gangal <[email protected]>

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

Signed-off-by: Harshit Gangal <[email protected]>

* test: refactor test

Signed-off-by: Harshit Gangal <[email protected]>

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

Signed-off-by: Harshit Gangal <[email protected]>

* test: separate expectation for mysql and vitess result

Signed-off-by: Harshit Gangal <[email protected]>

* test: fix test expectation

Signed-off-by: Harshit Gangal <[email protected]>

* feat: added bitnum bind variable and test

Signed-off-by: Harshit Gangal <[email protected]>

* proto: vtadmin side update

Signed-off-by: Harshit Gangal <[email protected]>

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

Signed-off-by: Harshit Gangal <[email protected]>

* added license header

Signed-off-by: Harshit Gangal <[email protected]>

Co-authored-by: Manan Gupta <[email protected]>
Signed-off-by: Vicent Marti <[email protected]>

* 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 <[email protected]>
Signed-off-by: Vicent Marti <[email protected]>

* cherry-pick: fix bad merge

Signed-off-by: Vicent Marti <[email protected]>

Co-authored-by: rsajwani <[email protected]>
Co-authored-by: Andres Taylor <[email protected]>
Co-authored-by: Kushal Kumar <[email protected]>
Co-authored-by: Manan Gupta <[email protected]>
Co-authored-by: Manan Gupta <[email protected]>
Co-authored-by: Florent Poinsard <[email protected]>
Co-authored-by: Harshit Gangal <[email protected]>
Co-authored-by: Rohit Nayak <[email protected]>
Co-authored-by: Dirkjan Bussink <[email protected]>
This was referenced Nov 2, 2022
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GuptaManan100 GuptaManan100 GuptaManan100 left review comments

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

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

Assignees
No one assigned
Labels
Component: Query Serving Type: Internal Cleanup
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@systay @deepthi @harshit-gangal @GuptaManan100 @frouioui