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
Make the AST visitor faster #7701
Conversation
Signed-off-by: Andres Taylor <andres@planetscale.com>
Signed-off-by: Andres Taylor <andres@planetscale.com>
Signed-off-by: Andres Taylor <andres@planetscale.com>
Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>
Signed-off-by: Andres Taylor <andres@planetscale.com>
Signed-off-by: Andres Taylor <andres@planetscale.com>
👋 any hints as for what this PR is expected to do? (no description in main comment) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Parser optimizations are on 🔥
I'm not familiar with the code and the changes are hard for me to follow. But approving based on the detailed PR comment and based on successful tests. Advising to wait for review by someone with more insights.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So amazing 🔥
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very neat! clean up the checked in benchmark and let's merge this!
Signed-off-by: Andres Taylor <andres@planetscale.com>
Signed-off-by: Andres Taylor <andres@planetscale.com>
return g.ptrToBasicMethod(t, ptrToType, gen) | ||
}) | ||
default: | ||
panic(fmt.Sprintf("%T", ptrToType)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: we can do log.fatal here, or is this intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1. If there's a good reason to panic, please document it.
Signed-off-by: Andres Taylor <andres@planetscale.com>
Description
Walking the query tree, either all of it or parts of it, is something the the planner does a number of times. To make the actual traversal of the tree as efficient as possible is therefor something that would benefit many parts of Vitess.
That is what this PR sets out to do. The old
sqlparser.Walk
method was implemented using theRewriter
framework. That is not a perfect match, so we had to do a little work to fit them together.This new implementation of the
sqlparser.Walk
functionality does three things differently, compared to the old on.panic
/recover
, like theRewrite
method does.Walk
visitor doesn't support replacing parts of the tree, we can use simpler code and not have to keep state to know which field needs to be replaced.Together, this all leads to some nice improvements. Here are some microbenchmark results of just walking the AST:
On average, walking the these tree takes about half the time it used to.
If we instead benchmark the vtgate planbuilder, which is a heavy user of
sqlparser.Walk
, we get:Overall some nice improvements on both time and memory pressure.
Impacted Areas in Vitess
Components that this PR will affect: