chore: update Repartition DisplayAs to indicate maintained sort order #18673

ruchirK · 2025-11-13T15:37:40Z

Previously, it was not obvious reading the plan diagram when a Repartition operator maintained sortedness by virtue of having a single input partition even if preserve_sort order was false. This commit makes the implicit sortedness preservation explicit in the plan diagram. This commit does not change anything for the case when preserve sort order is false.

Which issue does this PR close?

Closes Display sort order in Hash Repartition #18594

Are these changes tested?

Yes

Are there any user-facing changes?

No

datafusion/core/tests/dataframe/mod.rs

gabotechs · 2025-11-13T16:14:47Z

Taking a look at this one shortly

gabotechs

I have to say I'm not fully convinced about this change. The extra added information is redundant with information already present in the plan display, as it's not really adding anything new, and it can make the plans more verbose without adding any real information.

I'd love to weight opinions of other contributors though.

gabotechs · 2025-11-13T19:25:58Z

datafusion/physical-plan/src/repartition/mod.rs

                if self.preserve_order {
                    write!(f, ", preserve_order=true")?;
+                } else if input_partition_count <= 1 {
+                    write!(f, ", maintains_sort_order=true")?;


I get the impression that this maintains_sort_order here might be confusing, as it's not a real property of RepartitionExec, and instead is something artificially derived from other properties that are already displayed, and therefore, redundant.

I agree -- this was confusing at first. Reading @ruchirK 's explanation in the description makes sense, but I think it is somewhat subtle. Maybe we can add some comments to the code to make it clearer.

I'll leave a suggestion

datafusion/physical-plan/src/repartition/mod.rs

ruchirK · 2025-11-14T01:58:18Z

I have to say I'm not fully convinced about this change. The extra added information is redundant with information already present in the plan display, as it's not really adding anything new, and it can make the plans more verbose without adding any real information.

I'd love to weight opinions of other contributors though.

Thank you for the thoughtful and prompt review @gabotechs ! My understanding of the original task was that in some cases it might be helpful to display some redundant information to make plan diagrams easier to parse at a glance.

The way I see it, there's two related questions here.

Should we add some new text to the plan diagram for cases when the RepartitionExec operator preserves sortedness? There's at least 3 answers here.
a. No, we already display input_partitions=1 which is sufficient.
b. Yes, but only in the implicit case. Display maintains_sort_order=true when there's only 1 partition. If preserve_order is true just display that, and don't show maintains_sort_order. That's the intended behavior of this PR.
c. Yes, but go further. Fully remove preserve_sort_order=true and only display maintains_sort_order. This would, as mentioned in the gh issue, conflate the implicit and explicit decisions.
When sortedness is implicitly maintained in the single partition case, should we show something in the plan diagram indicating what the implicitly maintained sort order is? Note that this is a little bit independent of question 1 and this was my intent with the sort_exprs change.

My personal take is to generally agree with you that these changes are not super big usability wins. I'd love to here @gene-bordegaray 's take as well. For now, I'll update the PR to remove the sort_exprs change and then we can go from there.

gabotechs · 2025-11-14T15:33:00Z

I think at this point there's a lot of subjectivity here.

I personally prefer a less cluttered display that does not contain redundant information. Following that same rule I'd also expect other plans like FilterExec or CoalesceBatchesExec to display wether they are implicitly maintaining the input ordering or not, but we don't seem to be doing that here, and I agree that it would make the cluttering even worst.

My point of view is that adding the extra information proposed in this PR is making the plan slightly less readable and slightly more confusing, so I cannot in good conscience give a +1 to this myself, but I'm not the only one reading plans here, and I don't have a strong opinion, so would love to hear opinions from other contributors and agree to what others think is best. Maybe @adriangb or @Jefffrey?

datafusion/core/tests/physical_optimizer/enforce_distribution.rs

adriangb · 2025-11-15T21:16:39Z

I agree I wouldn’t want to see this in every plan but if the input is sorted it would be helpful to see that RepartitionExec preserves that.

Jefffrey · 2025-11-17T12:53:22Z

I agree I wouldn’t want to see this in every plan but if the input is sorted it would be helpful to see that RepartitionExec preserves that.

I'm of a similar mind here; as the PR currently is, the added information isn't particularly helpful for a lot of the plans I'm seeing changed

gabotechs

I still maintain the personal preference of https://github.com/apache/datafusion/pull/18673/files#r2524672693, but as some people find this useful then +1 on my side. Thanks for this work!

parquet-testing

testing

alamb

Thank you @ruchirK and @gabotechs

I have a few suggestions, but I also think this PR could be merged as is if we want

alamb · 2025-11-21T15:36:44Z

datafusion/physical-plan/src/repartition/mod.rs

                if self.preserve_order {
                    write!(f, ", preserve_order=true")?;
+                } else if input_partition_count <= 1 {
+                    write!(f, ", maintains_sort_order=true")?;


I agree -- this was confusing at first. Reading @ruchirK 's explanation in the description makes sense, but I think it is somewhat subtle. Maybe we can add some comments to the code to make it clearer.

I'll leave a suggestion

datafusion/physical-plan/src/repartition/mod.rs

Previously, it was not obvious reading the plan diagram when a Repartition operator maintained sortedness by virtue of having a single input partition even if preserve_sort order was false. This commit makes the implicit sortedness preservation explicit in the plan diagram. This commit does not change anything for the case when preserve sort order is false, or if the input was not originally sorted.

ruchirK · 2025-11-21T18:45:01Z

@alamb thanks for the review I've followed all of your suggestions!

gabotechs · 2025-11-22T10:56:19Z

Thanks @ruchirK for the PR and @adriangb, @Jefffrey, @NGA-TRAN and @alamb for the input!

github-actions bot added core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) physical-plan Changes to the physical-plan crate labels Nov 13, 2025

ruchirK commented Nov 13, 2025

View reviewed changes

datafusion/core/tests/dataframe/mod.rs Show resolved Hide resolved

gabotechs reviewed Nov 13, 2025

View reviewed changes

ruchirK force-pushed the repartition-display branch from 26a1c57 to 4cd99c3 Compare November 14, 2025 02:42

NGA-TRAN reviewed Nov 14, 2025

View reviewed changes

datafusion/core/tests/physical_optimizer/enforce_distribution.rs Outdated Show resolved Hide resolved

NGA-TRAN reviewed Nov 14, 2025

View reviewed changes

datafusion/core/tests/physical_optimizer/enforce_distribution.rs Outdated Show resolved Hide resolved

ruchirK force-pushed the repartition-display branch 4 times, most recently from 679c489 to 9539583 Compare November 19, 2025 18:05

gabotechs approved these changes Nov 20, 2025

View reviewed changes

parquet-testing Show resolved Hide resolved

testing Show resolved Hide resolved

alamb mentioned this pull request Nov 20, 2025

Andrew Lamb Weekly-ish Open Source plan - 2025-11-17 #18711

Open

44 tasks

ruchirK force-pushed the repartition-display branch from 9539583 to b63f77a Compare November 20, 2025 18:44

alamb approved these changes Nov 21, 2025

View reviewed changes

ruchirK force-pushed the repartition-display branch from b63f77a to 31d954e Compare November 21, 2025 18:44

gabotechs added this pull request to the merge queue Nov 22, 2025

Merged via the queue into apache:main with commit 5f22722 Nov 22, 2025
32 checks passed

chore: update Repartition DisplayAs to indicate maintained sort order #18673

chore: update Repartition DisplayAs to indicate maintained sort order #18673

Conversation

ruchirK commented Nov 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

Uh oh!

gabotechs commented Nov 13, 2025

Uh oh!

gabotechs left a comment

Choose a reason for hiding this comment

Uh oh!

gabotechs Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

alamb Nov 21, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ruchirK commented Nov 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gabotechs commented Nov 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adriangb commented Nov 15, 2025

Uh oh!

Jefffrey commented Nov 17, 2025

Uh oh!

gabotechs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

alamb Nov 21, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ruchirK commented Nov 21, 2025

Uh oh!

gabotechs commented Nov 22, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

ruchirK commented Nov 13, 2025 •

edited

Loading

ruchirK commented Nov 14, 2025 •

edited

Loading

gabotechs commented Nov 14, 2025 •

edited

Loading