-
Notifications
You must be signed in to change notification settings - Fork 665
feat: Introduce support for EXPLAIN/ANALYZE #284
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
Conversation
Pull Request Test Coverage Report for Build 443038537
💛 - Coveralls |
f236c37
to
05eda1a
Compare
Hello @Dandandan @nickolay Can you please take a look? Thanks! |
Thanks for your PR @ovr ! Looking good. I left some review comments |
@Dandandan, Thank for your comments, I've updated PR. Added |
62592d5
to
8e0e691
Compare
src/ast/mod.rs
Outdated
pub enum Statement { | ||
// EXPLAIN | ||
Explain { | ||
level: Option<ExplainLevel>, |
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.
I think it's not an enum but rather a combination of options.
see: https://www.postgresql.org/docs/9.1/sql-explain.html
I think all the following are correct:
EXPLAIN
EXPLAIN ANALYZE
EXPLAIN VERBOSE
EXPLAIN ANALYZE VERBOSE
So maybe this would be a better encoding?
Explain {
analyze: bool
verbose: bool
}
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.
Yeap, you are right.
I checked What combintations work with PostgreSQL:
EXPLAIN VERBOSE SELECT * from test;
EXPLAIN ANALYSE SELECT * from test;
EXPLAIN ANALYZE VERBOSE SELECT * from test;
What doesnt work:
EXPLAIN VERBOSE ANALYZE SELECT * from test;
Find very funny thing, that they support ANALYSE
, but it's not mentioned inside docs.
I've updated my PR.
8e0e691
to
5b2658c
Compare
5b2658c
to
f0801db
Compare
Thanks a lot @ovr for your contribution! |
Hello!
Thanks