Add internal repartition metrics#21152
Open
gene-bordegaray wants to merge 2 commits into
Open
Conversation
github-actions
Bot
added
documentation
gene-bordegaray
changed the title
Contributor
|
I have some concerns about these low-level (kernel-profiling) metrics, so I’m sharing a few suggestions. (Not trying to block this, given this is useful to solve real problems—just offering additional perspectives and possible improvements.) Metrics are typically used for query tuning at the application level, while these low-level ones are mainly for internal debugging and are less frequently used. They may also introduce execution overhead that’s hard to observe, and bring maintenance overhead. In general, it might be better to keep metrics that directly help application tuning, are frequently used, or are difficult to capture with external profilers. I suspect some of them can be directly observed with profilers/flamegraphs, maybe they can be simplified? Additionally, we could consider introducing a new analyze level |
Contributor
|
run benchmarks |
|
🤖 Benchmark running (GKE) | trigger File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Details
Resource Usagetpch — base (merge-base)
tpch — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Details
Resource Usagetpcds — base (merge-base)
tpcds — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Details
Resource Usageclickbench_partitioned — base (merge-base)
clickbench_partitioned — branch
File an issue against this benchmark runner |
Contributor
Author
Ya I agree and am trying to be wary about this as well. I don't not see any changes on benches but like may be hard to catch microregressions. I am using these and they are quite useful (and very convenient compared to a profiling tool like xctrace). Maybe these could be kept at the 'dev' level of analyze output? |
Contributor
I was thinking I'll open an issue for this idea. |
Contributor
Author
hey following up on this, I wasn't able to find an issue but I would be willing to for these metrics. I think they are worth the additional level 👍 |
Contributor
|
@gene-bordegaray Thanks, really appreciate it! Please go ahead. |
gene-bordegaray
force-pushed
the
gene.bordegaray/2026/03/repartition_metrics
branch
from
cd5514f to
769ad4f
Compare
gene-bordegaray
changed the title
gene-bordegaray
force-pushed
the
gene.bordegaray/2026/03/repartition_metrics
branch
from
769ad4f to
fe6c854
Compare
github-actions
Bot
added
physical-expr
|
Thank you for opening this pull request! Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch). Details |
gene-bordegaray
force-pushed
the
gene.bordegaray/2026/03/repartition_metrics
branch
from
fe6c854 to
1b1ee57
Compare
github-actions
Bot
removed
physical-expr
gene-bordegaray
force-pushed
the
gene.bordegaray/2026/03/repartition_metrics
branch
from
1b1ee57 to
5924eaf
Compare
github-actions
Bot
added
physical-expr
Contributor
Author
|
@2010YOUY01 I have added this behind an 'internal' metrics level. I decided to keep this in this PR as the introduction of the "internal" metrics level and repartition being the first use case seemed fitting and not too large of a scope. These metrics have led me to find an improvement in the oeprator on fanned out small batches here: #22159 Let me know what you think on the way it is introduced, also not torn to do it in two PRs if preferred |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Rationale for this change
RepartitionExecspends time in different phases, but the existing metrics only expose broad timings. More granular timings are useful when investigating repartition bottlenecks without adding defaultEXPLAIN ANALYZEnoise or runtime overhead.What changes are included in this PR?
internaldatafusion.explain.analyze_levelfor low-level development metrics.MetricBuilder::if_enabled.RepartitionExectiming metrics as the first consumer of this internal metrics level.AnalyzeExecmetric types through physical plan protobuf serialization and uses them during execution so metrics are registered consistently.Are these changes tested?
Yes. Added coverage for conditional metric registration,
RepartitionExecinternal metric registration,EXPLAIN ANALYZEoutput, and physical plan protobuf roundtrip behavior.Are there any user-facing changes?
Yes. Users can set
datafusion.explain.analyze_level = 'internal'to include low-level repartition timing metrics inEXPLAIN ANALYZE. The defaultdevoutput is unchanged.