Repository navigation
fix: preserve outer join equivalences for null rows - #26042
Prajwal-k-tech wants to merge 5 commits into
Conversation
|
I pushed a regression fix for the CI failure: zero-column batches now preserve their row count, with a focused Full-join test. GitHub marked the new fork workflow runs as |
|
I also addressed the semver bot feedback by restoring the original public |
|
Thanks @Prajwal-k-tech Which of these Prs would you like us to review first?
I think it will be eaier to focus and get one done / in rather than trying to do a bunch in parallel |
kumarUjjawal
left a comment
There was a problem hiding this comment.
Thank you @Prajwal-k-tech
Left few comments please take a look.
| let expressions = class.into_iter().filter(|expr| { | ||
| !is_volatile(expr) | ||
| && expr | ||
| .evaluate(null_batch) |
There was a problem hiding this comment.
Planning now runs every non-volatile expression in an equivalence class, including user and third-party ScalarUDFs, on a synthetic all-NULL batch; it does this every time join properties are recomputed, and a panic inside evaluate is not caught.
| && expr | ||
| .evaluate(null_batch) | ||
| .and_then(|value| value.into_array(1)) | ||
| .is_ok_and(|array| array.null_count() == 1) |
There was a problem hiding this comment.
array.null_count() == 1 reads the physical null buffer, so arrays with only logical nulls report 0 and their valid equivalences are dropped.
| /// that may no longer hold after an outer join. | ||
| fn with_null_preserving_expressions(self, null_batch: &RecordBatch) -> Self { | ||
| let classes = self.classes.into_iter().filter_map(|class| { | ||
| let expressions = class.into_iter().filter(|expr| { |
There was a problem hiding this comment.
The filter keeps only members that evaluate to NULL, so it drops members that still agree on null-extended rows because they evaluate to the same non-NULL value.
| )?); | ||
| } | ||
| JoinType::Full => { | ||
| let batch = null_batch(join_schema)?; |
There was a problem hiding this comment.
null_batch allocates a new schema and one null array per output column on every outer-join property computation, even when the nullable side has no equivalence classes.**
| schema | ||
| .fields() | ||
| .iter() | ||
| .map(|field| Field::new(field.name(), field.data_type().clone(), true)) |
There was a problem hiding this comment.
null_batch rebuilds each field with Field::new(name, type, true), which drops field metadata such as extension-type annotations.
| statement ok | ||
| CREATE TABLE outer_join_sort_right (a BIGINT, b BIGINT) AS VALUES (2, 2), (3, 3); | ||
|
|
||
| query III |
There was a problem hiding this comment.
The regression tests cover only LEFT and FULL OUTER JOIN; the new JoinType::Right branch (class.rs:874) and the cross-join path are never exercised.
| Ok(()) | ||
| } | ||
|
|
||
| use datafusion_expr::Operator; |
There was a problem hiding this comment.
The new test full_join_equivalences_support_empty_schema was inserted between the test module's use lines, leaving use datafusion_expr::Operator; stranded after a function.
There was a problem hiding this comment.
The public EquivalenceGroup::join (no schema) keeps the old unsound outer-join behavior with no doc warning or deprecation, so external callers still get wrong equivalences.
|
Thanks for the detailed review. I pushed the follow-up to the existing fork branch in commits
Validation: |
| /// Replace input columns with NULL without evaluating the expression. | ||
| fn null_extended_expr(expr: &PhysicalExprRef) -> Option<PhysicalExprRef> { | ||
| if expr.downcast_ref::<Column>().is_some() { | ||
| return Some(Arc::new(Literal::new(ScalarValue::Null))); |
There was a problem hiding this comment.
Could we preserve the type information, or drop these expressions when we cannot prove they remain equal? A regression test with a type-sensitive UDF would help cover this case.
|
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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #26042 +/- ##
==========================================
+ Coverage 82.66% 82.72% +0.06%
==========================================
Files 1147 1147
Lines 446357 448289 +1932
Branches 446357 448289 +1932
==========================================
+ Hits 368971 370843 +1872
+ Misses 54997 54900 -97
- Partials 22389 22546 +157 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…ache#26570) ## Which issue does this PR close? - None. This PR only adds tests, so there is no bug or feature issue to close. ## Rationale for this change The planner skips sorts and keeps streaming execution when it can prove that the data already has the needed order. It decides this from what each plan node knows about its output: which orderings hold, which columns are constant, and which columns are equal. Several of these decisions have no test today: - a union that stays sorted when each side has a cast of the sort key in a different column, - a constant from a filter that survives a sort with a limit or a cross join, - the order in which GROUP BY keys are matched against the input's orderings, - an order-preserving repartition over an unbounded input that keeps a cast of its sort key sorted. Two open PRs show why this matters. apache#26459 and apache#26042 change how constants pass through outer joins. Cross join derives its output facts as if it were a full outer join, so both would make cross-join queries sort on columns that are still constant, and no existing test fails. The cross-join cases added here fail with either PR merged in. Without tests, a change to how the planner tracks these facts could add sorts, change the aggregation mode, or make a streaming query fail to plan, and nothing would catch it. This PR pins the plans that main produces today. ## What changes are included in this PR? A new `datafusion/sqllogictest/test_files/equivalence_properties.slt`, with one comment per case that says which planner decision it pins: - A union whose sides each have a cast of `a` in a different column is still known to be sorted by `a`. - `GROUP BY d, c, b, a` over data sorted by `(a, b)` and by `c` finds `c, a, b` sorted and runs in partially sorted mode on them. - A filter constant survives an `ORDER BY ... LIMIT`, so an outer sort that starts with that constant is removed. - A cross join keeps the constants of its left input. - A cross join keeps a right-side constant set by an equality filter. - Over an unbounded input, an order-preserving repartition keeps a cast of its sort key sorted, so a window partitioned by that cast needs no sort. In `datafusion/core/tests/physical_optimizer/enforce_distribution.rs`, a new test checks that sort-merge join keys are realigned to a range partitioning when the second of two orderings of an unbounded input matches it, not only the first. The existing `ordered_stream` helper now takes a list of orderings so the new test can reuse it. ## What is the testing strategy for this PR? This PR adds tests only. All new cases pass on the current main branch, and the new sqllogictest file gives the same result over repeated runs. ## Are there any user-facing changes? No.
…#26459) ## Which issue does this PR close? - Closes apache#26283. ## Rationale for this change A query that orders by a column that is constant on the probe side of a join fails to plan: ```text SanityCheckPlan caused by Error during planning: Plan: ["SortPreservingMergeExec: [k@0 DESC]", " HashJoinExec: ..."] does not satisfy order requirements: [k@0 DESC]. Child-0 order: [] ``` `EquivalenceClass::try_with_offset` builds a new class from the expressions and does not copy `constant`. `EquivalenceGroup::join` uses it for every class of the right side, so the join output forgets that `k` is constant. `EnsureRequirements` removes the sort because `k` is constant below the join, and `SanityCheckPlan` then rejects the plan because the join output no longer says so. ## What changes are included in this PR? - `try_with_offset` keeps the class's constant value. - `EquivalenceGroup::join` clears the right side's constants for `Left` and `Full` joins. Unmatched rows pad that side with NULLs, so a constant there no longer holds. Without this, copying the constant would drop a sort that is needed. The first version of this PR added a sort in `EnsureRequirements` wherever the requirement could not be proven. Following review, it now fixes the join instead. The plan for apache#26283 then has no sort at all, instead of a sort on a constant column. The same problem exists the other way round: the left side's constants are kept for `Right` and `Full` joins, where that side is padded with NULLs too. For example, this returns `5` before the NULLs: ```sql CREATE TABLE t(a INT) AS VALUES (1), (2), (3); CREATE TABLE u(a INT) AS VALUES (9), (10), (1); SELECT s.k, u.a FROM (SELECT a, 5 AS k FROM t) s RIGHT JOIN u ON s.a = u.a ORDER BY s.k DESC NULLS FIRST; ``` This PR does not change the left side, because apache#26042 already rebuilds the classes of the side that is padded with NULLs, which drops these constants as well. ## What is the testing strategy for this PR? - `class.rs`: a unit test that a right-side constant stays constant after `Inner` and `Right` joins, and not after `Left` and `Full` joins. - `order.slt`: both queries from apache#26283, with results and `EXPLAIN`. Without the change, both fail to plan. - `order.slt`: a `LEFT JOIN` where the probe-side constant is padded with NULLs. If the constant is not cleared, the sort on it is dropped and the rows come back in the wrong order. ## Are there any user-facing changes? Queries like the one in apache#26283 plan and run instead of failing. There are no API changes.
|
Thanks for the contribution! I am closing this under DataFusion's AI Dump policy. This has the hallmarks of AI code that was not reviewed by the author: an initial version that evaluated expressions against a synthetic all-NULL batch at plan time, bullet-list replies ending in command runs, and reviewer questions left unanswered. If you would like to continue, please open a focused PR in your own words that you have reviewed and understand, and we are happy to help with review. |
Which issue does this PR close?
coalesce/CASE#26036.Rationale for this change
After a LEFT, RIGHT, or FULL OUTER JOIN, the nullable side can contain rows whose columns were extended with NULLs. An equivalence such as
b = coalesce(a, 0)is true on the input rows but does not remain true for those null-extended rows. Retaining it can lead to incorrect ordering and LIMIT results.What changes are included in this PR?
What is the testing strategy for this PR?
cargo test -p datafusion-sqllogictest --test sqllogictests -- joinscargo test -p datafusion-physical-expr(1,685 passed, 2 ignored; 13 doctests passed)RUST_BACKTRACE=1 cargo test -p datafusion(passed)RUST_BACKTRACE=1 cargo test -p datafusion-cli(passed)cargo clippy --all-targets --all-features -- -D warnings(passed)cargo +1.99.0 fmt --all -- --checkandgit diff --check(passed)This contribution was prepared with AI assistance.
Are there any user-facing changes?
This fixes incorrect query results for affected outer joins. There are no API changes.