fix: preserve only the maintained input ordering in joins by lyne7-sc · Pull Request #25053 · apache/datafusion

lyne7-sc · 2026-09-08T07:30:26Z

Which issue does this PR close?

Closes Incorrect join ordering properties can cause required sorts to be removed #25052.

Rationale for this change

SortMergeJoin and HashJoin can incorrectly append the other input's ordering to the maintained input's ordering. With duplicate probe keys, matching rows repeat for each probe row, so the combined ordering is not guaranteed.

This can cause required sorts to be removed, producing incorrect results for ORDER BY, ORDER BY with LIMIT, and ranking window functions.

What changes are included in this PR?

Remove the special cases that append the other input's ordering in join_equivalence_properties and calculate_join_output_ordering.
Preserve the maintained input's ordering, including the existing column index adjustment for the right input.
Update unit test expectations and physical plan snapshots to reflect the corrected ordering properties.

What is the testing strategy for this PR?

Update unit tests for both ordering helpers and add regression cases in joins.slt

Are there any user-facing changes?

Affected queries retain required sorts and return correct results, while joins with provably safe output ordering continue to avoid unnecessary sorts.

The public join_equivalence_properties function now requires a has_filter argument. This is a breaking API change: downstream callers must indicate whether an additional join filter is present.

codecov-commenter · 2026-09-08T07:57:28Z

Codecov Report

❌ Patch coverage is 91.78571% with 46 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.79%. Comparing base (06aa131) to head (e703447).

Files with missing lines	Patch %	Lines
.../physical-expr/src/equivalence/properties/joins.rs	91.29%	10 Missing and 31 partials ⚠️
...on/physical-expr/src/equivalence/properties/mod.rs	93.02%	0 Missing and 3 partials ⚠️
...on/physical-plan/src/joins/sort_merge_join/exec.rs	93.75%	1 Missing ⚠️
...ion/physical-plan/src/joins/symmetric_hash_join.rs	92.30%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #25053      +/-   ##
==========================================
+ Coverage   82.77%   82.79%   +0.01%     
==========================================
  Files        1147     1147              
  Lines      450909   451374     +465     
  Branches   450909   451374     +465     
==========================================
+ Hits       373252   373693     +441     
+ Misses      54934    54925       -9     
- Partials    22723    22756      +33

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

sunchao

Thanks for fixing the incorrect join ordering. I compared 1f5ff8c with its parent f96892a: all 512 bounded native oracle cases passed on this head, including inner/outer joins, null handling, and both sort directions; 224 failed before the fix. The explicit SortExec runtime-bypass reproducer is also fixed. I found a streaming-progress regression for a constraint-proven safe ordering and a small LIMIT coverage gap; details inline.

sunchao · 2026-09-09T00:37:57Z

-            } else {
-                result.add_orderings(right_oeq_class);
-            }
+            result.add_orderings(right_oeq_class);


[P2] Preserve constraint-proven ordering for unique-build streaming joins

Unconditionally dropping the build-side suffix can make a previously productive streaming query wait indefinitely, even when the input metadata proves that suffix safe. I reproduced this with an inner hash join:

Bounded build input: physical PRIMARY KEY(k) metadata, ordering (k, v), and rows (1, 100), (2, 200).

Unbounded probe input: ordered by k, emitting several k = 1 rows and then remaining pending.

Required output: ORDER BY probe.k, build.v LIMIT 1.

The parent returns a row immediately. This head inserts PartialSortExec with TopK(fetch=1) and common_prefix_length=1; it times out and cannot emit until the key changes or the input ends. If the equal-key group never ends, it never returns. Finite-input and equivalent-key-suffix controls still return correctly.

The uniqueness premise is present on the physical build source and survives all physical optimizer passes. Since each probe key matches at most one build row, build.v is constant within each equal-probe.k group, making this particular suffix valid for all inputs satisfying those guarantees.

Please preserve the narrowly proven inner-join case and add an unbounded regression test. This needs both a suitable build unique/primary key and a maintained ordering that determines the corresponding join keys; a primary-key check alone would not justify restoring the old blanket join_suffix rule.

Thanks for pointing this out. The inner join case can indeed retain the ordering with the right uniqueness guarantees. I also noticed that probe-preserving outer joins can retain it when the maintained ordering determines all join keys and there is no additional join filter. I’ve updated the ordering inference for these cases and added tests verifying that LIMIT returns even when the unbounded probe stream remains pending within the same key group.

sunchao · 2026-09-09T00:37:57Z

+query II
+SELECT l.column1, r.column2
+FROM duplicate_join_left l JOIN duplicate_join_right r ON l.column1 = r.column1
+ORDER BY l.column1, r.column2 LIMIT 2;


Consider adding a LIMIT case that fails without the fix

This exact LIMIT 2 query also passes on the parent commit: a SortExec: TopK(fetch=2) remains above the sort-merge join. The new plain ORDER BY, ranking, and ordered-source right-join tests do reproduce the bug, but this particular test does not establish the LIMIT impact.

A direct physical-plan regression can cover it: keep an explicit SortExec(left.k, right.v).with_fetch(Some(2)) above an inner SortMergeJoinExec, with the left input sorted by k and the right input sorted by (k, v). With two left rows for k=1 and right values 100, 200, the parent returns 100, 200 while this head correctly returns 100, 100. That also exercises the runtime sort-skipping path used by direct physical-plan consumers.

Thanks. I’ve added the suggested test and verified that it fails on the pre-fix commit (f96892a) and passes on the updated branch.

github-actions · 2026-09-10T02:29:33Z

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 v55.1.0 (current)
       Built [  38.190s] (current)
     Parsing datafusion v55.1.0 (current)
      Parsed [   0.023s] (current)
    Building datafusion v55.1.0 (baseline)
       Built [  37.714s] (baseline)
     Parsing datafusion v55.1.0 (baseline)
      Parsed [   0.023s] (baseline)
    Checking datafusion v55.1.0 -> v55.1.0 (no change; assume patch)
     Checked [   0.429s] 223 checks: 223 pass, 31 skip
     Summary no semver update required
    Finished [  78.214s] datafusion
    Building datafusion-physical-expr v55.1.0 (current)
       Built [  18.050s] (current)
     Parsing datafusion-physical-expr v55.1.0 (current)
      Parsed [   0.034s] (current)
    Building datafusion-physical-expr v55.1.0 (baseline)
       Built [  18.037s] (baseline)
     Parsing datafusion-physical-expr v55.1.0 (baseline)
      Parsed [   0.034s] (baseline)
    Checking datafusion-physical-expr v55.1.0 -> v55.1.0 (no change; assume patch)
     Checked [   0.251s] 223 checks: 222 pass, 1 fail, 0 warn, 31 skip

--- failure function_parameter_count_changed: pub fn parameter count changed ---

Description:
A publicly-visible function now takes a different number of parameters.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#fn-change-arity
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.50.0/src/lints/function_parameter_count_changed.ron

Failed in:
  datafusion_physical_expr::equivalence::join_equivalence_properties now takes 9 parameters instead of 7, in /home/runner/work/datafusion/datafusion/datafusion/physical-expr/src/equivalence/properties/joins.rs:34

     Summary semver requires new major version: 1 major and 0 minor checks failed
    Finished [  40.977s] datafusion-physical-expr
    Building datafusion-physical-plan v55.1.0 (current)
       Built [  23.708s] (current)
     Parsing datafusion-physical-plan v55.1.0 (current)
      Parsed [   0.120s] (current)
    Building datafusion-physical-plan v55.1.0 (baseline)
       Built [  23.871s] (baseline)
     Parsing datafusion-physical-plan v55.1.0 (baseline)
      Parsed [   0.129s] (baseline)
    Checking datafusion-physical-plan v55.1.0 -> v55.1.0 (no change; assume patch)
     Checked [   0.474s] 223 checks: 223 pass, 31 skip
     Summary no semver update required
    Finished [  53.310s] datafusion-physical-plan
    Building datafusion-sqllogictest v55.1.0 (current)
       Built [  60.783s] (current)
     Parsing datafusion-sqllogictest v55.1.0 (current)
      Parsed [   0.012s] (current)
    Building datafusion-sqllogictest v55.1.0 (baseline)
       Built [  60.872s] (baseline)
     Parsing datafusion-sqllogictest v55.1.0 (baseline)
      Parsed [   0.013s] (baseline)
    Checking datafusion-sqllogictest v55.1.0 -> v55.1.0 (no change; assume patch)
     Checked [   0.068s] 223 checks: 223 pass, 31 skip
     Summary no semver update required
    Finished [ 124.188s] datafusion-sqllogictest

sunchao

Follow-up review of a82ba03. The three remaining findings are attached inline.

sunchao · 2026-09-10T03:55:58Z

+    build.constraints().iter().any(|constraint| {
+        let (Constraint::PrimaryKey(indices) | Constraint::Unique(indices)) = constraint;
+        !indices.is_empty()
+            && indices.iter().all(|&index| {


[P2] Correct projected constraints before restoring ordering

Dropping or reordering a build-side primary key can incorrectly transfer its uniqueness constraint to a duplicate join key. This guard then restores unsafe ordering: LIMIT 2 returns [10,20] instead of [10,10]. The updated head fails 16 projection cases that the previous revision passed. The constraint-projection defect predates this PR; the new exception reintroduces the original wrong-result behavior. Correct or discard these projected constraints before trusting them.

Fixed in 0a4a474. Constraint projection now maps each constrained input column to its directly projected output column, preserving the constraint type. If any column of a composite key is dropped, the entire constraint is discarded. Added regressions for primary-key reordering, incomplete composite unique constraints, and the join limit case returning 10, 10.

sunchao · 2026-09-10T03:55:58Z

+                // UNIQUE can contain repeated NULLs. Without null-equality
+                // information, only non-null UNIQUE columns prove uniqueness.
+                if matches!(constraint, Constraint::Unique(_)) && field.is_nullable() {
+                    return false;


[P2] Preserve nullable UNIQUE proofs for ordinary equality

With NullEqualsNothing, NULL build keys cannot match, so nullable UNIQUE(k) still guarantees at most one matching build row. Rejecting it here introduces PartialSortExec for ORDER BY probe.k, build.v LIMIT 1. The base returns immediately; the updated head waits indefinitely when the probe-key group never finishes. Finite controls pass. Include null-equality semantics in this proof.

Fixed in 13c2969. The uniqueness proof now uses the join’s actual null-equality semantics: nullable unique is accepted with NullEqualsNothing and rejected with NullEqualsNull. I’ve added a regression test covering both ordering decisions, cache invalidation, and correct ORDER BY with LIMIT results.

sunchao · 2026-09-10T03:55:58Z

+                on.iter().any(|(probe_key, build_key)| {
+                    build_key.eq(&column) && probe_exprs.contains(probe_key)
+                })


[P2] Recognize computed keys fixed by the ordering prefix

Exact expression membership misses ON build.k = probe.k + 1 when the probe is ordered by k. Equal probe.k values determine the same primary-key build row, making its suffix safe. Nevertheless, ORDER BY probe.k, build.k, build.v LIMIT 1 gains PartialSortExec and stalls on an unfinished group; the base returns immediately. Check whether fixing the ordering expressions also fixes the computed join key.

Fixed in 13c2969. The ordering proof now recognizes computed join keys determined by the probe ordering, while excluding volatile expressions. I’ve added regression tests for unbounded-stream progress and consistent normalization of ordering expressions and join keys.

sunchao

Follow-up review of 1622fd1. The earlier three findings are fixed; two remaining findings are attached inline.

sunchao · 2026-09-16T15:45:06Z

+        let probe_key_is_fixed = |key: &PhysicalExprRef| {
+            !is_volatile(key) && group.is_expr_constant(key).is_some()


[P2] Check volatility before normalizing join keys

A filter such as p = volatile_flip(p) records an equivalence that normalizes the join key p + volatile_flip(p) into p + p. This volatility check then misses the function and incorrectly accepts the ordering suffix. The native reproducer returns [10,20] for LIMIT 2 instead of [10,10]. The previous PR revision passed this case; the current exception reintroduces the original failure. Preserve the original key's volatility before normalization.

thanks for the follow-up review. I reproduced both issues locally. The proof now checks volatility before normalization on both sides and accounts for probe-key nullability under null-safe equality. Added regressions for the incorrect LIMIT results and the unbounded-probe stall.

sunchao · 2026-09-16T15:45:06Z

+                if matches!(constraint, Constraint::Unique(_))
+                    && field.is_nullable()
+                    && null_equality == NullEquality::NullEqualsNull
+                {
+                    return false;


[P2] Account for nonnullable probe keys under null-safe equality

A nullable UNIQUE build key still guarantees at most one match when the corresponding probe key cannot be NULL, even with NullEqualsNull. This guard rejects that safe case. For an ordered unbounded probe, ORDER BY probe.k, build.v LIMIT 1 returns immediately on base 5747e87 but gains PartialSortExec on this head and waits indefinitely if the key group never finishes. The finite control passes. Include probe-key nullability in the uniqueness proof.

sunchao

Follow-up review of 019e9c8. Both previously posted findings are fixed; two remaining native streaming-progress regressions are attached inline. Both reproduce against current base a2b8093.

sunchao · 2026-09-17T16:16:40Z

+    if build.constraints().is_empty() || build.oeq_class().is_empty() {
+        return Ok(OrderingEquivalenceClass::default());


[P2] Preserve suffix ordering when the probe prefix is unique

The build-constraint requirement overlooks a safe case: when the probe ordering contains its primary key, each ordering group contains one probe row, so ordered build matches cannot restart within that group. With a PK-constrained probe emitting one row then remaining pending, and four ordered duplicate build matches, ORDER BY probe.k, build.v LIMIT 1 returns immediately on current base a2b8093. Here, both HashJoin and SortMergeJoin gain PartialSortExec and time out waiting for more input. Finite controls return correctly. Include probe-side uniqueness as an alternative proof.

Updated to handle unique probe keys, including nullable UNIQUE when NULLs cannot match.

sunchao · 2026-09-17T16:16:40Z

+    for ordering in probe.oeq_class().iter() {
+        // Within a group of equal ordering values, these expressions are
+        // constant. Keep this assumption local to the ordering proof.
+        let mut group = probe.eq_group().clone();
+        for sort in ordering {
+            let expr = probe.eq_group().normalize_expr(Arc::clone(&sort.expr));
+            group.add_constant(ConstExpr::from(expr));


[P2] Consider combined probe orderings for composite keys

Checking each stored ordering separately misses a valid combined prefix. A probe independently ordered by [k] and [v] is also ordered by [k,v], which can determine a build PRIMARY KEY(k,v). Neither singleton passes this proof, so ORDER BY probe.k, probe.v, build.payload LIMIT 1 gains PartialSortExec and stalls when the probe remains pending. The identical native plan returns immediately on current base a2b8093; finite controls pass on both revisions. Also evaluate valid concatenated prefixes, retaining the suffix only for the prefix actually proved.

Updated to combine independent probe orderings and find sufficient prefixes without including unrelated ordering columns.

sunchao

Follow-up review of 976250b. Both exact reproducers from the previous review now pass; the remaining computed-key streaming regression is attached inline.

sunchao · 2026-10-07T16:23:09Z

+        let (ordering, _) = probe.find_longest_permutation(&keys)?;
+        orderings.extend(LexOrdering::new(ordering));


[P2] Include determining columns when selecting computed-key prefixes

With independent probe orderings [extra], [a], [b], a build PRIMARY KEY(k1,k2), and join keys a % 2 = k1 and b % 2 = k2, this search considers only the modulo expressions. It misses the sufficient [a,b] prefix: equal a,b values determine one build row, making its payload constant. The full combined candidate includes unrelated extra and cannot establish ORDER BY a,b,payload. In the native reproducer, LIMIT 1 returns immediately on base a2b8093 but gains PartialSortExec and waits indefinitely here when the probe remains pending. Finite controls pass. Include ordered input expressions determining the computed keys, then apply the existing suffix-safety proof.

Updated in 31d1284. The search now considers join-key subexpressions and removes redundant prefix items, while retaining the existing safety checks.

fix: preserve only the maintained input ordering in joins

1f5ff8c

github-actions Bot added physical-expr Changes to the physical-expr crates sqllogictest SQL Logic Tests (.slt) physical-plan Changes to the physical-plan crate labels Sep 8, 2026

sunchao reviewed Sep 9, 2026

View reviewed changes

fix: preserve join output ordering only when guaranteed

0a68e2e

github-actions Bot added the core Core DataFusion crate label Sep 10, 2026

recompute hash join properties when updating the filter

a82ba03

github-actions Bot added the auto detected api change Auto detected API change label Sep 10, 2026

sunchao reviewed Sep 10, 2026

View reviewed changes

lyne7-sc and others added 3 commits September 10, 2026 22:37

fix: correctly project primary key and unique constraints

0a4a474

account for null equality and computed keys in join ordering proofs

13c2969

Merge branch 'main' into fix/join-output-ordering

1622fd1

sunchao reviewed Sep 16, 2026

View reviewed changes

lyne7-sc added 2 commits September 17, 2026 22:34

Merge branch 'main' into fix/join-output-ordering

4e2eabf

fix: account for volatility and nullability in join ordering proofs

019e9c8

sunchao reviewed Sep 17, 2026

View reviewed changes

preserve join ordering for unique probe keys and combined prefixes

976250b

sunchao reviewed Oct 7, 2026

View reviewed changes

lyne7-sc added 2 commits October 9, 2026 23:08

fix: consider join key subexpressions for probe ordering

31d1284

Merge branch 'main' into fix/join-output-ordering

e703447

		let probe_key_is_fixed = \|key: &PhysicalExprRef\| {
		!is_volatile(key) && group.is_expr_constant(key).is_some()

		if build.constraints().is_empty() \|\| build.oeq_class().is_empty() {
		return Ok(OrderingEquivalenceClass::default());

		let (ordering, _) = probe.find_longest_permutation(&keys)?;
		orderings.extend(LexOrdering::new(ordering));

Conversation

lyne7-sc commented Sep 8, 2026 • 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?

What is the testing strategy for this PR?

Are there any user-facing changes?

Uh oh!

codecov-commenter commented Sep 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sunchao left a comment

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!

github-actions Bot commented Sep 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sunchao left a comment

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!

Choose a reason for hiding this comment

Uh oh!

sunchao left a comment

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!

sunchao left a comment

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!

sunchao left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lyne7-sc commented Sep 8, 2026 •

edited

Loading

codecov-commenter commented Sep 8, 2026 •

edited

Loading

github-actions Bot commented Sep 10, 2026 •

edited

Loading