Removed incorrect union check in enforce_sorting and updated tests #18661

gene-bordegaray · 2025-11-13T02:18:56Z

Which issue does this PR close?

Closes Preserving sort on UnionExec inputs instead of introducing a suboptimal top-level sort #18380.
Closes Teach UnionExec to require its inputs sorted #9898.

Rationale for this change

There was an overly aggressive condition enforce_sorting rule was not handling UnionExec correctly. This conditions assumed that Unions did not maintain order causing SortExec nodes to be removed and then eventually added at a higher level, less efficiently.

What changes are included in this PR?

I removed this condition that now has changed the logic to properly take into account UnionExec's ability to maintain input ordering.

Are these changes tested?

Yes, previously failing tests were ignored and now are unignored and passing.

Are there any user-facing changes?

No

gene-bordegaray · 2025-11-13T11:30:18Z

Cc: @NGA-TRAN - leaving this in draft while I finish writing documentation on the enforce sorting rule, think this is the fix though 😄

NGA-TRAN

LGTM

@rgehan : this will fix your request #18380
Can you help review it?

@gene-bordegaray : You may not need any long document explaining this unless you really want to do so. a short description is good enough

cc @alamb @2010YOUY01

NGA-TRAN · 2025-11-13T22:28:15Z

datafusion/core/tests/dataframe/mod.rs

+        AggregateExec: mode=Partial, gby=[id@0 as id], aggr=[], ordering_mode=Sorted
+          UnionExec
+            DataSourceExec: file_groups={1 group: [[{testdata}/alltypes_tiny_pages.parquet]]}, projection=[id], output_ordering=[id@0 ASC NULLS LAST], file_type=parquet
+            SortExec: expr=[id@0 ASC NULLS LAST], preserve_partitioning=[false]


Great push down

NGA-TRAN · 2025-11-13T22:30:56Z

datafusion/core/tests/physical_optimizer/enforce_sorting.rs

-        SortExec: expr=[nullable_col@0 ASC], preserve_partitioning=[false]
-          DataSourceExec: file_groups={1 group: [[x]]}, projection=[nullable_col, non_nullable_col], file_type=parquet
-        SortExec: expr=[nullable_col@0 ASC], preserve_partitioning=[false]
-          DataSourceExec: file_groups={1 group: [[x]]}, projection=[nullable_col, non_nullable_col], file_type=parquet


Can you explain why this is no longer needed? Because the input and output are now the same?

Yes, I was confused at first but this is what it means.

NGA-TRAN · 2025-11-13T22:33:13Z

datafusion/physical-optimizer/src/enforce_sorting/mod.rs

-            || !plan.maintains_input_order()[idx]
-            || is_union(plan)
-        {
+        } else if physical_ordering.is_none() || !plan.maintains_input_order()[idx] {


Wow. This is simple.
I wonder why this is_union is here in the first place 🤔

All the tests pass and @rgehan's reproducer now also pass means this is likely the right work

Yes I was very confused as well when stepping through the logic. The only thing I could think of is if it was thought that UnionExec was never able to maintain input order but it is implemented for the operator.

Interesting though since this is quoted from the documentation online about the operator: "UnionExec combines multiple inputs with the same schema by concatenating the partitions. It does not mix or copy data within or across partitions. Thus if the input partitions are sorted, the output partitions of the union are also sorted."

Very nice analysis

Wow. This is simple. I wonder why this is_union is here in the first place 🤔

I guess when implementing optimizer rules, it’s more common to be conservative than comprehensive if not sure, to avoid bugs.

gene-bordegaray · 2025-11-14T02:43:02Z

LGTM

@rgehan : this will fix your request #18380 Can you help review it?

@gene-bordegaray : You may not need any long document explaining this unless you really want to do so. a short description is good enough

cc @alamb @2010YOUY01

Ok sounds good. More doing so for my own understanding and to give to others but shouldn't hold back this PR 😄

rgehan

LGTM, although I'm not knowledgeable enough to understand the impact of dropping this is_union() condition.

In addition to fixing the tests I previously added, I can confirm it also fixes my production use-cases 👍

2010YOUY01

Amazing work, thank you!

2010YOUY01 · 2025-11-15T06:42:19Z

datafusion/physical-optimizer/src/enforce_sorting/mod.rs

-            || !plan.maintains_input_order()[idx]
-            || is_union(plan)
-        {
+        } else if physical_ordering.is_none() || !plan.maintains_input_order()[idx] {


Wow. This is simple. I wonder why this is_union is here in the first place 🤔

I guess when implementing optimizer rules, it’s more common to be conservative than comprehensive if not sure, to avoid bugs.

alamb

Thank you @gene-bordegaray @rgehan, and @NGA-TRAN

I reviewed this plan and code carefully and I agree there is no reason to force the sort after a union if the inputs are already sorted.

alamb · 2025-11-17T13:59:36Z

datafusion/core/tests/dataframe/mod.rs

    assert_snapshot!(
        union_with_mix_of_presorted_and_explicitly_resorted_inputs_impl(false).await?,
-        @r#"
+        @r"


I reviewed this plan carefully and it looks good to me

alamb · 2025-11-20T18:20:37Z

Thanks again @gene-bordegaray

gene-bordegaray · 2025-11-20T18:27:37Z

Thanks again @gene-bordegaray

Glad to help 🫡

…pache#18661) ## Which issue does this PR close?  - Closes apache#18380. - Closes apache#9898. ## Rationale for this change  There was an overly aggressive condition enforce_sorting rule was not handling UnionExec correctly. This conditions assumed that Unions did not maintain order causing SortExec nodes to be removed and then eventually added at a higher level, less efficiently. ## What changes are included in this PR?  I removed this condition that now has changed the logic to properly take into account UnionExec's ability to maintain input ordering. ## Are these changes tested?  Yes, previously failing tests were ignored and now are unignored and passing. ## Are there any user-facing changes?   No

Removed incorreect union check in enforce_sorting and updated tests

506b460

github-actions bot added optimizer Optimizer rules core Core DataFusion crate labels Nov 13, 2025

NGA-TRAN approved these changes Nov 13, 2025

View reviewed changes

gene-bordegaray marked this pull request as ready for review November 14, 2025 02:45

alamb changed the title ~~Removed incorreect union check in enforce_sorting and updated tests~~ Removed incorrect union check in enforce_sorting and updated tests Nov 14, 2025

rgehan approved these changes Nov 14, 2025

View reviewed changes

This was referenced Nov 14, 2025

Andrew Lamb Weekly-ish Open Source plan - 2025-11-03 #18486

Closed

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

Closed

2010YOUY01 approved these changes Nov 15, 2025

View reviewed changes

alamb approved these changes Nov 17, 2025

View reviewed changes

alamb added this pull request to the merge queue Nov 20, 2025

Merged via the queue into apache:main with commit 1833093 Nov 20, 2025
35 checks passed

Removed incorrect union check in enforce_sorting and updated tests #18661

Removed incorrect union check in enforce_sorting and updated tests #18661

Uh oh!

Conversation

gene-bordegaray 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?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

gene-bordegaray commented Nov 13, 2025

Uh oh!

NGA-TRAN left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gene-bordegaray commented Nov 14, 2025

Uh oh!

rgehan left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

2010YOUY01 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb commented Nov 20, 2025

Uh oh!

Uh oh!

gene-bordegaray commented Nov 20, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gene-bordegaray commented Nov 13, 2025 •

edited

Loading

NGA-TRAN left a comment •

edited

Loading

NGA-TRAN Nov 13, 2025 •

edited

Loading

rgehan left a comment •

edited

Loading

alamb left a comment •

edited

Loading