-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Add metrics for UnnestExec #8482
Conversation
2b3f19e
to
b332399
Compare
b332399
to
8c9f89e
Compare
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.
Thank you for this contributuion @simonvandel -- very nice 👍
let formatted = arrow::util::pretty::pretty_format_batches(&results) | ||
.unwrap() | ||
.to_string(); | ||
assert_contains!(&formatted, "elapsed_compute="); |
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.
👍
} | ||
|
||
#[derive(Clone, Debug)] | ||
struct UnnestMetrics { |
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 has some overlap with BaselineMetrics
, it might be able to be combined if you wanted to reduce the code size somewhat
https://docs.rs/datafusion/latest/datafusion/physical_plan/metrics/struct.BaselineMetrics.html
self.num_output_batches, | ||
self.num_output_rows, | ||
self.unnest_time, | ||
produced {} output batches containing {} rows in {}", |
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.
👍
Which issue does this PR close?
Closes #8481.
Rationale for this change
See #8481
What changes are included in this PR?
Adds a new
UnnestMetrics
struct to collect metrics.Are these changes tested?
Yes, added a DF test.
I'm not sure if there are better places to have this kind of test.
I saw that there are similar explain-tests for SQL. However, unnest is not yet implemented for SQL.
Are there any user-facing changes?
Yes, additional metrics now show up in
EXPLAIN ANALYZE
queries.