Skip to content

fix: preserve join predicates during reordering - #27066

Merged
jayzhan211 merged 1 commit into
apache:mainfrom
Phoenix500526:issue/26058
Nov 8, 2026
Merged

fix: preserve join predicates during reordering#27066
jayzhan211 merged 1 commit into
apache:mainfrom
Phoenix500526:issue/26058

Conversation

@Phoenix500526

@Phoenix500526 Phoenix500526 commented Nov 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Important

Fork CI setup: For pull requests from a fork to main, enable workflows in your fork's Actions tab before you push.
See the fork CI setup guide.

Which issue does this PR close?

Rationale for this change

Join reordering can drop predicates and return incorrect results. For example:

SELECT *
FROM (VALUES (1)) AS a(x)
CROSS JOIN (VALUES (1)) AS k(x)
JOIN (VALUES (1)) AS b(x)
  ON a.x = b.x
 AND a.x + k.x = b.x;

This query should return no rows because 1 + 1 = 1 is false. However, EliminateCrossJoin can reorder the inputs into (a JOIN b) CROSS JOIN k and drop the second condition, incorrectly returning one row.

Rebuilding joins can also overwrite individual keys’ NULL comparison semantics when ordinary equality and NULL-safe equality are mixed, causing missing or extra rows.

What changes are included in this PR?

  • Preserve original join predicates and retain conditions that rebuilt joins cannot enforce as residual filters.
  • Track ordinary and NULL-safe join keys separately, removing predicates only when rebuilt joins enforce the same comparison semantics.
  • Convert the existing NULL semantics unit test into four table-driven scenarios.

What is the testing strategy for this PR?

Add five regression cases in joins.slt covering CASE predicate preservation, mixed NULL semantics in both directions, NULL-safe residual predicates, and ordinary equality on columns already joined with NULL-safe equality.

Validation passed:

  • Optimizer unit, integration, and documentation tests.
  • SQL logic tests selected by joins.slt.
  • cargo fmt --all.
  • cargo clippy --all-targets --all-features -- -D warnings.

Are there any user-facing changes?

Affected queries now preserve all join conditions and their NULL comparison semantics, returning the correct rows. No public API changes.

@github-actions github-actions Bot added optimizer Optimizer rules sqllogictest SQL Logic Tests (.slt) labels Nov 3, 2026
@Phoenix500526
Phoenix500526 marked this pull request as ready for review November 3, 2026 23:30
@Phoenix500526

Phoenix500526 commented Nov 3, 2026

Copy link
Copy Markdown
Contributor Author

CC @alexy

@Phoenix500526 Phoenix500526 mentioned this pull request Nov 4, 2026
Closed
@Phoenix500526
Phoenix500526 force-pushed the issue/26058 branch 3 times, most recently from 0ec42b9 to 66e050c Compare November 7, 2026 15:23
@jayzhan211
jayzhan211 self-requested a review November 8, 2026 00:43
jayzhan211
jayzhan211 approved these changes Nov 8, 2026
View reviewed changes

@jayzhan211 jayzhan211 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Phoenix500526 thanks for fixing both issues with one rule! Fork CI was skipped here, so I ran the full sqllogictest suite locally on 66e050c: everything passes, and all five new joins.slt cases fail without the fix.

Optional cleanup: in eliminate_cross_join.rs around line 307, the find_inner_join doc comment still describes a single key set. It could say that ordinary keys are tried first for each right input, then NULL-safe keys. The matched keys go into all_join_keys or all_null_equal_join_keys, and the join takes their NullEquality.

Comment thread datafusion/optimizer/src/eliminate_cross_join.rs Resolved
@jayzhan211 jayzhan211 mentioned this pull request Nov 8, 2026
Closed
jayzhan211
jayzhan211 requested changes Nov 8, 2026
View reviewed changes

@jayzhan211 jayzhan211 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Phoenix500526 thanks for applying the cleanup! It removed the last non-test caller of JoinKeySet::is_empty (join_key_set.rs around line 108), so cargo clippy -p datafusion-optimizer --all-targets -- -D warnings now fails with method `is_empty` is never used. That one's my miss, sorry. Fork CI skipped clippy, so it doesn't show up here. Adding #[cfg(test)], as len already has, fixes it; clippy and the join_key_set tests pass with it:

--- a/datafusion/optimizer/src/join_key_set.rs
+++ b/datafusion/optimizer/src/join_key_set.rs
@@ -105,6 +105,7 @@ impl JoinKeySet {
     }
 
     /// returns true if this set is empty
+    #[cfg(test)]
     pub fn is_empty(&self) -> bool {
         self.inner.is_empty()
     }

@Phoenix500526
fix: preserve join predicates during reordering
2b948e9 
Keep predicates that rebuilt joins cannot enforce so reordering does not
drop conditions. Preserve each key's NULL semantics to avoid missing or
introducing matches when ordinary and null-safe joins are mixed.

CLOSES apache#26058
@Phoenix500526

Phoenix500526 commented Nov 8, 2026

Copy link
Copy Markdown
Contributor Author

@Phoenix500526 thanks for applying the cleanup! It removed the last non-test caller of JoinKeySet::is_empty (join_key_set.rs around line 108), so cargo clippy -p datafusion-optimizer --all-targets -- -D warnings now fails with method `is_empty` is never used. That one's my miss, sorry. Fork CI skipped clippy, so it doesn't show up here. Adding #[cfg(test)], as len already has, fixes it; clippy and the join_key_set tests pass with it:

--- a/datafusion/optimizer/src/join_key_set.rs
+++ b/datafusion/optimizer/src/join_key_set.rs
@@ -105,6 +105,7 @@ impl JoinKeySet {
}

 /// returns true if this set is empty
  • #[cfg(test)]
    pub fn is_empty(&self) -> bool {
    self.inner.is_empty()
    }

Done

@jayzhan211
jayzhan211 added this pull request to the merge queue Nov 8, 2026
@jayzhan211

jayzhan211 commented Nov 8, 2026

Copy link
Copy Markdown
Contributor

Thanks @Phoenix500526 !

Merged via the queue into apache:main with commit 5191bcc Nov 8, 2026
44 checks passed
@Phoenix500526
Phoenix500526 deleted the issue/26058 branch November 8, 2026 03:18
@jayzhan211 jayzhan211 mentioned this pull request Nov 9, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jayzhan211 jayzhan211 jayzhan211 approved these changes

Assignees

No one assigned

Labels

optimizer Optimizer rules sqllogictest SQL Logic Tests (.slt) v57.0.0

Projects

None yet

Milestone

No milestone

2 participants

@Phoenix500526 @jayzhan211