Skip to content

Conversation

@2010YOUY01
Copy link
Contributor

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-cli

DataFusion CLI v51.0.0
> explain analyze select a+1, pow(a,2)
from generate_series(1,1000000) as t1(a);
+-------------------+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| plan_type         | plan                                                                                                                                                                                                                                                                                      |
+-------------------+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| Plan with Metrics | ProjectionExec: expr=[a@0 + 1 as t1.a + Int64(1), power(CAST(a@0 AS Float64), 2) as pow(t1.a,Int64(2))], metrics=[output_rows=1.00 M, elapsed_compute=32.37ms, output_bytes=15.3 MB, output_batches=123, expr_eval_time_0=8.27ms, expr_eval_time_1=23.82ms]                               |
|                   |   RepartitionExec: partitioning=RoundRobinBatch(14), input_partitions=1, metrics=[output_rows=1.00 M, elapsed_compute=780.87µs, output_bytes=7.7 MB, output_batches=123, spill_count=0, spilled_bytes=0.0 B, spilled_rows=0, fetch_time=1.19ms, repartition_time=1ns, send_time=186.54µs] |
|                   |     ProjectionExec: expr=[value@0 as a], metrics=[output_rows=1.00 M, elapsed_compute=148.62µs, output_bytes=7.7 MB, output_batches=123, expr_eval_time_0=4.12µs]                                                                                                                         |
|                   |       LazyMemoryExec: partitions=1, batch_generators=[generate_series: start=1, end=1000000, batch_size=8192], metrics=[output_rows=1.00 M, elapsed_compute=1.01ms, output_bytes=7.7 MB, output_batches=123]                                                                              |
|                   |                                                                                                                                                                                                                                                                                           |
+-------------------+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
1 row(s) fetched.

Each expressions' evaluation time are tracked individually in expr_eval_time_*. expr_eval_time_0 is for a+1, and expr_eval_time_1 is for pow(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?

  1. Moved metrics module from datafusion-execution crate to datafusion-physical-expr-common. (I optimistically think it's enough to move to execution crate previously, however there are many execution utilities live in physical-expr-common, so we have to further move it down in the dependency tree)
  2. Added a struct to hold each expression's evaluation time: ExpressionEvaluatorMetrics
  3. Put Option<ExpressionEvaluatorMetrics> inside Projector, and change the execution accordingly.

Are these changes tested?

Yes, added several slts

Are there any user-facing changes?

No

@github-actions github-actions bot added physical-expr Changes to the physical-expr crates sqllogictest SQL Logic Tests (.slt) execution Related to the execution crate physical-plan Changes to the physical-plan crate labels Dec 22, 2025
.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)
Copy link
Member

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?

Copy link
Contributor Author

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
Copy link
Member

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 ?

Copy link
Contributor Author

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<'_>> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pub fn scoped_timer(&self, index: usize) -> Option<ScopedTimerGuard<'_>> {
#[inline]
pub fn scoped_timer(&self, index: usize) -> Option<ScopedTimerGuard<'_>> {

Copy link
Contributor Author

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;
Copy link
Member

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.

Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

execution Related to the execution crate physical-expr Changes to the physical-expr crates physical-plan Changes to the physical-plan crate sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants