sqlparser: Refactor out servenv and inject everywhere #14822
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is a refactor similar as was done with collations, to remove the servenv dependency from sqlparser and to make sqlparser usable entirely with no global state for other dependencies. This means we now create a sqlparser instance with the appropriate settings instead of modifying global configuration flags for things like the version and for truncation settings. Signed-off-by: Dirkjan Bussink <[email protected]>
dbussink
requested review from
deepthi,
mattlord,
rohit-nayak-ps,
derekperkins,
harshit-gangal,
systay,
shlomi-noach,
GuptaManan100,
frouioui,
arthurschreiber,
ajm188 and
notfelineit
as code owners
dbussink
added
Type: Internal Cleanup
Component: General
dbussink
commented
Dec 19, 2023
| func (pq *ParsedQuery) MarshalJSON() ([]byte, error) { | ||
| return json.Marshal(TruncateForUI(pq.Query)) | ||
| return json.Marshal(pq.Query) |
There was a problem hiding this comment.
I have removed the truncation here. The JSON serialization for this was only used in tests and there we can also work fine without the truncation. Since it's not used outside of tests it looks like, it's safe here to remove. Keeping this was really annoying with how global state is affecting this, so removing it is far more trivial.
| @@ -339,7 +339,7 @@ | |||
| } | |||
| ], | |||
| "FullQuery": "update d set foo = 'foo' where `name` in ('a', 'b') limit :#maxLimit", | |||
| "WhereClause": "where `name` in ('a', 'b')" | |||
| "WhereClause": " where `name` in ('a', 'b')" | |||
There was a problem hiding this comment.
These cases change since they use the JSON serialization mentioned earlier and a side-effect of TruncateForUI was that it also trimmed whitespaces. We now dropped that so the original text is still here. It has no functional change though.
| @@ -104,7 +107,6 @@ type vcursorImpl struct { | |||
| topoServer *topo.Server | |||
| logStats *logstats.LogStats | |||
| collation collations.ID | |||
| collationEnv *collations.Environment | |||
There was a problem hiding this comment.
Removed it from here since I realized with the parser, we can always get it from the executor already anyway. Keeping it consistent between the parser and collation env this way.
shlomi-noach
approved these changes
Dec 19, 2023
This reduces the dependency on the binlog package and json logic if not strictly needed for the mysql package. Signed-off-by: Dirkjan Bussink <[email protected]>
GuptaManan100
approved these changes
Dec 19, 2023
frouioui
approved these changes
Dec 19, 2023
This was referenced Jan 12, 2024
ejortegau
pushed a commit
to slackhq/vitess
that referenced
this pull request
Jan 24, 2024
Signed-off-by: Dirkjan Bussink <[email protected]> Signed-off-by: Eduardo J. Ortega U. <[email protected]>
systay
pushed a commit
that referenced
this pull request
Jul 22, 2024
…here (#3997) * cherry pick of 14822 Signed-off-by: Dirkjan Bussink <[email protected]> * boost: Fix injecting sqlparser everywhere as well Signed-off-by: Dirkjan Bussink <[email protected]> --------- Signed-off-by: Dirkjan Bussink <[email protected]> Co-authored-by: Dirkjan Bussink <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a refactor similar as was done with collations, to remove the servenv dependency from sqlparser and to make sqlparser usable entirely with no global state for other dependencies.
This means we now create a sqlparser instance with the appropriate settings instead of modifying global configuration flags for things like the version and for truncation settings.
Related Issue(s)
Part of #14717, follow up to #14781
Checklist