-
-
Notifications
You must be signed in to change notification settings - Fork 13
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
Enhance explain #146
base: dev
Are you sure you want to change the base?
Enhance explain #146
Conversation
It depends on sqlparser apache/datafusion-sqlparser-rs#1426. Awaiting the next release of the library. |
Should this be based off of #140? |
c6f38a0
to
cc4262f
Compare
Ok(r) | ||
})?; | ||
|
||
Ok(rows.join("")) |
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.
This just adds a new row right?
Style::Postgres => { | ||
let mut output = format!("DuckDB Scan: {}\n", query); | ||
if state.analyze { | ||
let start_time = Instant::now(); | ||
set_search_path_by_pg()?; | ||
connection::execute(&query, [])?; | ||
let duration = start_time.elapsed(); | ||
output += &format!( | ||
"Execution Time: {:.3} ms\n", | ||
duration.as_micros() as f64 / 1_000.0 | ||
); | ||
} | ||
output | ||
} | ||
Style::Duckdb => { |
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.
This is cool. What do you see as the use case for supporting both styles? Is there anything the DuckDB style offers that can't be in a single style?
I don't want to increase cognitive load on the user. Ideally, they wouldn't need to pass any setting to EXPLAIN if they can avoid it. What do you think?
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.
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.
That looks super clean, I really like that. Great work, wow! We should document this in the docs/
in paradedb/paradedb.
Really nice stuff. I assume it works with (analyze) even without the ()? i.e. EXPLAIN ANALYZE
?
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.
Could you please provide output examples for all the style/options you are adding here? I.e. what the query looks like, and what the output looks like? I'd like to build a better understanding of what this will look like before we merge anything.
Also some tests, but I understand this PR isn't ready for review yet.
That looks really great. I think we should totally add this STYLE option :). Great idea! |
Ticket(s) Closed
EXPLAIN
Statement: Addanalyze
option, Supportstyle
option #124What
Why
How
Support the ANALYZE option: add a timer to record the actual execution time.
Support the STYLE option: Provide the query plan and execution time in DuckDB style.
Tests