-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat: Add per-expression evaluation timing metrics to ProjectionExec #19447
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
base: main
Are you sure you want to change the base?
Conversation
| .with_new_label("expr", label.into()) | ||
| .with_type(MetricType::DEV) | ||
| // Existing PhysicalExpr formatter is a bit verbose, so use simple name | ||
| .subset_time(format!("expr_eval_time_{idx}"), partition) |
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.
How about expr_[idx]_eval_time?
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.
fe31656 updated.
Indeed, it looks nicer.
| /// Create metrics for a collection of expressions. | ||
| /// | ||
| /// # Args | ||
| /// - expression_labels: unique identifier for each metric, so that the metric |
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.
Items for the other args ?
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.
updated a304cd8, thanks.
| } | ||
|
|
||
| /// Returns a timer guard for the expression at `index`, if present. | ||
| pub fn scoped_timer(&self, index: usize) -> Option<ScopedTimerGuard<'_>> { |
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.
| pub fn scoped_timer(&self, index: usize) -> Option<ScopedTimerGuard<'_>> { | |
| #[inline] | |
| pub fn scoped_timer(&self, index: usize) -> Option<ScopedTimerGuard<'_>> { |
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.
updated a304cd8
| @@ -32,7 +32,6 @@ pub mod cache; | |||
| pub mod config; | |||
| pub mod disk_manager; | |||
| pub mod memory_pool; | |||
| pub mod metrics; | |||
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.
Are there any user-facing changes?
No
This actually is a user-facing removal of a public item.
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.
Oh yeah, I forgot to mention this was moved in #19247, and hasn't went through any release, if we can make this PR before the next release, it wouldn't be a breaking change. Otherwise I will add the API change tag.
Which issue does this PR close?
Part of #18456
Rationale for this change
See issue for the rationale, this PR is implementing the 1st part of the issue.
Demo in
datafrusion-cliEach expressions' evaluation time are tracked individually in
expr_eval_time_*.expr_eval_time_0is fora+1, andexpr_eval_time_1is forpow(a,2).I chose number index because the pretty formatting for expressions are still a bit verbose now (like
[a@0 + 1 as t1.a + Int64(1))What changes are included in this PR?
datafusion-executioncrate todatafusion-physical-expr-common. (I optimistically think it's enough to move toexecutioncrate previously, however there are many execution utilities live inphysical-expr-common, so we have to further move it down in the dependency tree)ExpressionEvaluatorMetricsOption<ExpressionEvaluatorMetrics>insideProjector, and change the execution accordingly.Are these changes tested?
Yes, added several slts
Are there any user-facing changes?
No