Skip to content

refactor: one rule for the equivalence properties of a merge of partitions - #26643

Draft
jayzhan211 wants to merge 1 commit into
apache:mainfrom
jayzhan211:equivalence-merged-partitions
Draft

jayzhan211 wants to merge 1 commit into
apache:mainfrom
jayzhan211:equivalence-merged-partitions

Conversation

@jayzhan211

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

Operators that combine rows from several input partitions must forget the facts that hold only inside each
partition: constants whose value may differ between partitions, and, unless the operator keeps order, the input's
orderings. Today this rule is written out in four places, and the copies disagree when the input has a single
partition:

One input partition Several input partitions
RepartitionExec keeps everything drops per-partition constants, and orderings unless order-preserving
join-key alignment in EnforceDistribution keeps everything drops per-partition constants
SortPreservingMergeExec drops per-partition constants drops per-partition constants
CoalescePartitionsExec drops per-partition constants and orderings drops per-partition constants and orderings

With one input partition, both SortPreservingMergeExec and CoalescePartitionsExec pass the input stream
through, so nothing they drop has stopped being true.

What changes are included in this PR?

  • EquivalenceProperties::merged(input_partitions, keeps_order): with one input partition everything is kept;
    otherwise per-partition constants are dropped, and orderings too unless keeps_order.
  • The four places above call it.

What is the testing strategy for this PR?

  • A unit test for merged: one partition against two, with and without keeps_order.
  • A test that CoalescePartitionsExec over one sorted partition reports that ordering (it fails before this
    change) and over two does not.
  • Existing tests, including all sqllogictests, pass without changes; no plan changes.

Are there any user-facing changes?

SortPreservingMergeExec and CoalescePartitionsExec over a single input partition keep the facts of their
input, so the planner may skip a sort it does not need. Results do not change. New public method
EquivalenceProperties::merged.

@github-actions

Copy link
Copy Markdown

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
     Cloning apache/main
    Building datafusion-physical-expr v55.1.0 (current)
       Built [  48.805s] (current)
     Parsing datafusion-physical-expr v55.1.0 (current)
      Parsed [   0.059s] (current)
    Building datafusion-physical-expr v55.1.0 (baseline)
       Built [  33.663s] (baseline)
     Parsing datafusion-physical-expr v55.1.0 (baseline)
      Parsed [   0.058s] (baseline)
    Checking datafusion-physical-expr v55.1.0 -> v55.1.0 (no change; assume patch)
     Checked [   0.347s] 229 checks: 228 pass, 1 fail, 0 warn, 31 skip

--- failure function_missing: pub fn removed or renamed ---

Description:
A publicly-visible function cannot be imported by its prior path. A `pub use` may have been removed, or the function itself may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.51.0/src/lints/function_missing.ron

Failed in:
  function datafusion_physical_expr::intervals::utils::check_statistics_support, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/cf2444ffe425aa529f39ab196963d20d5a56340a/datafusion/physical-expr/src/intervals/utils.rs:46

     Summary semver requires new major version: 1 major and 0 minor checks failed
    Finished [  83.857s] datafusion-physical-expr
    Building datafusion-physical-optimizer v55.1.0 (current)
       Built [  45.463s] (current)
     Parsing datafusion-physical-optimizer v55.1.0 (current)
      Parsed [   0.021s] (current)
    Building datafusion-physical-optimizer v55.1.0 (baseline)
       Built [  45.883s] (baseline)
     Parsing datafusion-physical-optimizer v55.1.0 (baseline)
      Parsed [   0.022s] (baseline)
    Checking datafusion-physical-optimizer v55.1.0 -> v55.1.0 (no change; assume patch)
     Checked [   0.123s] 229 checks: 229 pass, 31 skip
     Summary no semver update required
    Finished [  92.699s] datafusion-physical-optimizer
    Building datafusion-physical-plan v55.1.0 (current)
       Built [  43.502s] (current)
     Parsing datafusion-physical-plan v55.1.0 (current)
      Parsed [   0.235s] (current)
    Building datafusion-physical-plan v55.1.0 (baseline)
       Built [  43.966s] (baseline)
     Parsing datafusion-physical-plan v55.1.0 (baseline)
      Parsed [   0.238s] (baseline)
    Checking datafusion-physical-plan v55.1.0 -> v55.1.0 (no change; assume patch)
     Checked [   0.686s] 229 checks: 229 pass, 31 skip
     Summary no semver update required
    Finished [  89.859s] datafusion-physical-plan

@github-actions github-actions Bot added the auto detected api change Auto detected API change label Oct 23, 2026
kumarUjjawal pushed a commit to kumarUjjawal/datafusion that referenced this pull request Oct 23, 2026
…tions (apache#26654)

## Which issue does this PR close?

- Closes apache#26646.

## Rationale for this change

When a sorted input with more than one partition goes through
`SortPreservingMergeExec`, the planner treats every ordering of the
input as still holding after the merge. Only the merge key ordering
actually holds. The other orderings are true within each partition, but
not across them.

Because of this, the planner can remove a sort that is still needed, and
the query returns rows out of order. With two files that are each sorted
by `a` and `b` and `target_partitions = 2`:

```sql
SELECT * FROM (SELECT a, b FROM t ORDER BY a LIMIT 10) ORDER BY b;
```

the plan has no sort for the outer `ORDER BY b`, and the rows come back
as `(1, 10), (1, 1), (2, 20), (3, 2)`.

## What changes are included in this PR?

- `SortPreservingMergeExec::compute_properties` clears the input
orderings when there is more than one input partition, before adding the
merge key ordering. This matches what `CoalescePartitionsExec` and
`RepartitionExec` already do. With a single input partition nothing is
merged, so the input orderings are kept as before.

The preserve-order `RepartitionExec` case mentioned in the issue is not
covered here.

Draft apache#26643 moves this logic into `EquivalenceProperties::merged`. If
that lands first, I'll update this PR to use `.merged(input_partitions,
false)` followed by `add_ordering`.

## What is the testing strategy for this PR?

- New sqllogictest case at the end of `order.slt`, using the
reproduction from the issue. It checks both the query result and the
physical plan, which must keep the `SortExec` above the
`SortPreservingMergeExec`. It fails on `main` with the wrong row order
from the issue, and passes with this change.
- The existing sqllogictests and the `datafusion-physical-plan` /
`datafusion-physical-optimizer` unit tests pass with no changes to
expected plans.

## Are there any user-facing changes?

Queries like the one above now return correctly ordered results. No
public API changes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto detected api change Auto detected API change optimizer Optimizer rules physical-expr Changes to the physical-expr crates physical-plan Changes to the physical-plan crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant