Skip to content
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.

Sign up for GitHub

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

Minor: Rename RepartitionMetrics::repartition_time to RepartitionMetrics::repart_time to match metric #11478

Merged
merged 1 commit into from
Jul 29, 2024
Merged

Minor: Rename RepartitionMetrics::repartition_time to RepartitionMetrics::repart_time to match metric #11478

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions datafusion/physical-plan/src/repartition/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -415,7 +415,7 @@ struct RepartitionMetrics {
/// Time in nanos to execute child operator and fetch batches
fetch_time: metrics::Time,
/// Time in nanos to perform repartitioning
repartition_time: metrics::Time,
repart_time: metrics::Time,
Copy link
Member

Choose a reason for hiding this comment

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

to me, the previous name is more readable. do we need this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My rationale was so that the field name matched the display name of this metric (which is repart_time)

(a few lines down from here)

Another potential would be to rename the metric to "repartition_time" which would be a user facing change (which is probably fine).

I think it would be nice to have them consistent. The exact value I have less of an opinion of

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, i agree that consistency is really desirable. Therefore this PR is a good change (I will approve).
I would prefer that we change the metric name though. Readable naming is as valuable for code as it is for external metrics. I know it's a breaking change, but I don't know of other way to improve the naming while avoiding the breakage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in #11703

/// Time in nanos for sending resulting batches to channels.
///
/// One metric per output partition.
Expand Down Expand Up @@ -449,7 +449,7 @@ impl RepartitionMetrics {

Self {
fetch_time,
repartition_time: repart_time,
repart_time,
send_time,
}
}
Expand Down Expand Up @@ -775,7 +775,7 @@ impl RepartitionExec {
context: Arc<TaskContext>,
) -> Result<()> {
let mut partitioner =
BatchPartitioner::try_new(partitioning, metrics.repartition_time.clone())?;
BatchPartitioner::try_new(partitioning, metrics.repart_time.clone())?;

// execute the child operator
let timer = metrics.fetch_time.timer();
Expand Down