Skip to content

Conversation

@2010YOUY01
Copy link
Contributor

Which issue does this PR close?

Preparation for #18456

Rationale for this change

  • Track individual expr's execution time in ProjectExec metrics (in EXPLAIN ANALYZE) #18456 requires this refactor. Now we have dependency physical-plan -> physical-expr -> common, and metrics module lives in physical-plan. We want metrics module to directly accessible from physical-expr crate, so they should be moved to common crate, and the first step for such refactor is to move metrics internal dependencies to common.
  • I believe this module is actually commonly needed through the workspace, so datafusion-common should be a good refactor destination.

What changes are included in this PR?

  • move physical-plan/src/metrics module to datafusion-common
  • Re-export those human-readable display utilities inside physical-plan, to prevent changing dependents.

Are these changes tested?

existing tests

Are there any user-facing changes?

No

@github-actions github-actions bot added core Core DataFusion crate common Related to common crate execution Related to the execution crate physical-plan Changes to the physical-plan crate labels Dec 4, 2025
Copy link
Member

@martin-g martin-g left a comment

Choose a reason for hiding this comment

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

Have you considered extracting this functionality to its own crate that depends on datafusion_common ?
Usually this helps reducing the build time of a project.

pub const TB: u64 = 1 << 40;
pub const GB: u64 = 1 << 30;
pub const MB: u64 = 1 << 20;
pub const KB: u64 = 1 << 10;
Copy link
Member

Choose a reason for hiding this comment

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

Those should have been named *iB, e.g. TiB, GiB, etc. since they are base-2, not base-10. But maybe it is too late now.

Copy link
Member

Choose a reason for hiding this comment

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

Or maybe it is not too late. This PR breaks the API anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is re-exported in the original place to keep the API stable, so unfortunately it's too late 🤦🏼

@2010YOUY01
Copy link
Contributor Author

Have you considered extracting this functionality to its own crate that depends on datafusion_common ? Usually this helps reducing the build time of a project.

From cargo build --release --timings, building datafusion-common is not a bottleneck right now, so I think it’s fine to keep everything in there for simplicity, we could consider breaking it after it grow larger someday.

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thanks @2010YOUY01 and @martin-g

@alamb alamb added this pull request to the merge queue Dec 5, 2025
Merged via the queue into apache:main with commit 482c6b8 Dec 5, 2025
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

common Related to common crate core Core DataFusion crate execution Related to the execution crate physical-plan Changes to the physical-plan crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants