You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "Prajwal-k-tech (via GitHub)" <gi...@apache.org> on 2026/10/04 22:20:41 UTC
[PR] fix: preserve outer join equivalences for null rows [datafusion]
Prajwal-k-tech opened a new pull request, #26042:
URL: https://github.com/apache/datafusion/pull/26042
## Which issue does this PR close?
- Closes #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?
- Filter nullable-side equivalence classes against a one-row all-NULL batch for the outer-join cases where that side may be null-extended.
- Ignore volatile expressions during this check and discard classes that are no longer meaningful.
- Add LEFT and FULL OUTER JOIN SQL regression cases, including the reported ORDER BY/LIMIT behavior.
## What is the testing strategy for this PR?
- Verified the new SQL cases fail on the unmodified code and pass with this change.
- `cargo test -p datafusion-sqllogictest --test sqllogictests -- joins`
- `cargo 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 -- --check` and `git 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: preserve outer join equivalences for null rows [datafusion]
Posted by "kumarUjjawal (via GitHub)" <gi...@apache.org>.
kumarUjjawal commented on code in PR #26042:
URL: https://github.com/apache/datafusion/pull/26042#discussion_r4356729815
##########
datafusion/physical-expr/src/equivalence/class.rs:
##########
@@ -902,6 +970,34 @@ impl EquivalenceGroup {
}
}
+/// Whether an expression tree references any input column.
+fn contains_column_reference(expr: &PhysicalExprRef) -> bool {
+ if expr.downcast_ref::<Column>().is_some() {
+ return true;
+ }
+ let children = expr.children();
+ if children.is_empty() {
+ return expr.downcast_ref::<Literal>().is_none();
+ }
+ children.into_iter().any(contains_column_reference)
+}
+
+/// 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)));
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: preserve outer join equivalences for null rows [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #26042:
URL: https://github.com/apache/datafusion/pull/26042#issuecomment-6360099246
<!-- semver-check-comment -->
Thank you for opening this pull request!
Reviewer note: [cargo-semver-checks](https://github.com/obi1kenobi/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>
<summary>Details</summary>
```
Cloning apache/main
Building datafusion-physical-expr v55.1.0 (current)
Built [ 23.081s] (current)
Parsing datafusion-physical-expr v55.1.0 (current)
Parsed [ 0.039s] (current)
Building datafusion-physical-expr v55.1.0 (baseline)
Built [ 21.954s] (baseline)
Parsing datafusion-physical-expr v55.1.0 (baseline)
Parsed [ 0.042s] (baseline)
Checking datafusion-physical-expr v55.1.0 -> v55.1.0 (no change; assume patch)
Checked [ 0.281s] 223 checks: 219 pass, 4 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.50.0/src/lints/function_missing.ron
Failed in:
function datafusion_physical_expr::proto::physical_expr_names, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/c33ee42c471ae50dfe92ebd75fd10336145ab968/datafusion/physical-expr/src/proto.rs:225
function datafusion_physical_expr::proto::extension_expr_node, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/c33ee42c471ae50dfe92ebd75fd10336145ab968/datafusion/physical-expr/src/proto.rs:118
function datafusion_physical_expr::proto::register_physical_expr, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/c33ee42c471ae50dfe92ebd75fd10336145ab968/datafusion/physical-expr/src/proto.rs:185
--- failure inherent_method_missing: pub method removed or renamed ---
Description:
A publicly-visible method or associated fn is no longer available under its prior name. It 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.50.0/src/lints/inherent_method_missing.ron
Failed in:
CastExpr::with_new_expr_and_target_field, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/c33ee42c471ae50dfe92ebd75fd10336145ab968/datafusion/physical-expr/src/expressions/cast.rs:292
Partitioning::keeps_keys_local, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/c33ee42c471ae50dfe92ebd75fd10336145ab968/datafusion/physical-expr/src/partitioning.rs:851
--- failure module_missing: pub module removed or renamed ---
Description:
A publicly-visible module cannot be imported by its prior path. A `pub use` may have been removed, or the module may have been renamed, removed, or made non-public.
ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.50.0/src/lints/module_missing.ron
Failed in:
mod datafusion_physical_expr::proto, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/c33ee42c471ae50dfe92ebd75fd10336145ab968/datafusion/physical-expr/src/proto.rs:18
--- failure trait_missing: pub trait removed or renamed ---
Description:
A publicly-visible trait cannot be imported by its prior path. A `pub use` may have been removed, or the trait 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.50.0/src/lints/trait_missing.ron
Failed in:
trait datafusion_physical_expr::proto::ExtensionExprFromProto, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/c33ee42c471ae50dfe92ebd75fd10336145ab968/datafusion/physical-expr/src/proto.rs:87
Summary semver requires new major version: 4 major and 0 minor checks failed
Finished [ 46.684s] datafusion-physical-expr
Building datafusion-sqllogictest v55.1.0 (current)
Built [ 67.234s] (current)
Parsing datafusion-sqllogictest v55.1.0 (current)
Parsed [ 0.013s] (current)
Building datafusion-sqllogictest v55.1.0 (baseline)
Built [ 69.140s] (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.073s] 223 checks: 222 pass, 1 fail, 0 warn, 31 skip
--- failure inherent_method_missing: pub method removed or renamed ---
Description:
A publicly-visible method or associated fn is no longer available under its prior name. It 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.50.0/src/lints/inherent_method_missing.ron
Failed in:
DataFusionSubstraitRoundTrip::with_optimization, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/c33ee42c471ae50dfe92ebd75fd10336145ab968/datafusion/sqllogictest/src/engines/datafusion_substrait_roundtrip_engine/runner.rs:60
Summary semver requires new major version: 1 major and 0 minor checks failed
Finished [ 138.610s] datafusion-sqllogictest
```
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: preserve outer join equivalences for null rows [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb closed pull request #26042: fix: preserve outer join equivalences for null rows
URL: https://github.com/apache/datafusion/pull/26042
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: preserve outer join equivalences for null rows [datafusion]
Posted by "Prajwal-k-tech (via GitHub)" <gi...@apache.org>.
Prajwal-k-tech commented on PR #26042:
URL: https://github.com/apache/datafusion/pull/26042#issuecomment-6007234693
No preference on my end; going numerically (#26040, then #26041, then #26042) works well. Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: preserve outer join equivalences for null rows [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #26042:
URL: https://github.com/apache/datafusion/pull/26042#issuecomment-6006302709
Thanks @Prajwal-k-tech
Which of these Prs would you like us to review first?
- https://github.com/apache/datafusion/pull/26041
- https://github.com/apache/datafusion/pull/26042
- https://github.com/apache/datafusion/pull/26040
I think it will be eaier to focus and get one done / in rather than trying to do a bunch in parallel
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: preserve outer join equivalences for null rows [datafusion]
Posted by "Prajwal-k-tech (via GitHub)" <gi...@apache.org>.
Prajwal-k-tech commented on PR #26042:
URL: https://github.com/apache/datafusion/pull/26042#issuecomment-6023901874
Thanks for the detailed review. I pushed the follow-up to the existing fork branch in commits `16a78c3` and `63cc845`.
- Outer-join pruning now replaces nullable-side columns with NULL structurally and retains equivalence groups whose rewritten expressions match, including expressions that produce the same non-NULL result. It does not evaluate physical expressions or ScalarUDFs during planning.
- The synthetic `RecordBatch` is removed, avoiding its allocations, physical-null-buffer assumptions, and reconstructed fields that lost metadata.
- The public `EquivalenceGroup::join` now applies the same conservative outer-join handling without requiring a schema; its docs describe that behavior.
- Added coverage for RIGHT JOIN, a no-key inner-join path, and expressions with matching non-NULL results after null extension. Moved the stranded import.
Validation: `cargo fmt --all -- --check`, workspace Clippy (`cargo clippy --all-targets --all-features -- -D warnings`), all 1,690 `datafusion-physical-expr` unit tests (1,688 passed, 2 ignored), and the focused `joins.slt` suite pass. The repository-wide extended tests could not complete because this checkout lacks `datafusion/testing/data`; a representative failure reports that `ARROW_TEST_DATA` is unset and asks for the test-data submodule. I have not counted that suite as passing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: preserve outer join equivalences for null rows [datafusion]
Posted by "Prajwal-k-tech (via GitHub)" <gi...@apache.org>.
Prajwal-k-tech commented on PR #26042:
URL: https://github.com/apache/datafusion/pull/26042#issuecomment-5987979339
I also addressed the semver bot feedback by restoring the original public `EquivalenceGroup::join` signature and using a crate-internal schema-aware helper for join planning. The updated head is `5f1cabc`; GitHub has created another set of `action_required` workflows for this commit. Could a maintainer approve the latest runs?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: preserve outer join equivalences for null rows [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #26042:
URL: https://github.com/apache/datafusion/pull/26042#issuecomment-6363486086
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/26042?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `86.88525%` with `16 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 82.72%. Comparing base ([`c3ef346`](https://app.codecov.io/gh/apache/datafusion/commit/c3ef346887412eeaeef644809c8fcfaf782f94e4?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`63cc845`](https://app.codecov.io/gh/apache/datafusion/commit/63cc845b0cf37c7299bef7abec5ba8a528ee9756?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 274 commits behind head on main.
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/26042?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [datafusion/physical-expr/src/equivalence/class.rs](https://app.codecov.io/gh/apache/datafusion/pull/26042?src=pr&el=tree&filepath=datafusion%2Fphysical-expr%2Fsrc%2Fequivalence%2Fclass.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9waHlzaWNhbC1leHByL3NyYy9lcXVpdmFsZW5jZS9jbGFzcy5ycw==) | 88.13% | [2 Missing and 12 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/26042?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [.../physical-expr/src/equivalence/properties/joins.rs](https://app.codecov.io/gh/apache/datafusion/pull/26042?src=pr&el=tree&filepath=datafusion%2Fphysical-expr%2Fsrc%2Fequivalence%2Fproperties%2Fjoins.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9waHlzaWNhbC1leHByL3NyYy9lcXVpdmFsZW5jZS9wcm9wZXJ0aWVzL2pvaW5zLnJz) | 50.00% | [0 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/26042?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ 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
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/26042?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: preserve outer join equivalences for null rows [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #26042:
URL: https://github.com/apache/datafusion/pull/26042#issuecomment-5985410611
<!-- semver-check-comment -->
Thank you for opening this pull request!
Reviewer note: [cargo-semver-checks](https://github.com/obi1kenobi/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>
<summary>Details</summary>
```
Cloning apache/main
Building datafusion-physical-expr v55.1.0 (current)
Built [ 29.470s] (current)
Parsing datafusion-physical-expr v55.1.0 (current)
Parsed [ 0.049s] (current)
Building datafusion-physical-expr v55.1.0 (baseline)
Built [ 28.554s] (baseline)
Parsing datafusion-physical-expr v55.1.0 (baseline)
Parsed [ 0.051s] (baseline)
Checking datafusion-physical-expr v55.1.0 -> v55.1.0 (no change; assume patch)
Checked [ 0.349s] 223 checks: 222 pass, 1 fail, 0 warn, 31 skip
--- failure method_parameter_count_changed: pub method parameter count changed ---
Description:
A publicly-visible method now takes a different number of parameters, not counting the receiver (self) parameter.
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/method_parameter_count_changed.ron
Failed in:
datafusion_physical_expr::equivalence::EquivalenceGroup::join takes 4 parameters in /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/705d2e66e6058a9682221d3e6bffabc69ba10efe/datafusion/physical-expr/src/equivalence/class.rs:812, but now takes 5 parameters in /home/runner/work/datafusion/datafusion/datafusion/physical-expr/src/equivalence/class.rs:816
Summary semver requires new major version: 1 major and 0 minor checks failed
Finished [ 59.637s] datafusion-physical-expr
Building datafusion-sqllogictest v55.1.0 (current)
Built [ 96.372s] (current)
Parsing datafusion-sqllogictest v55.1.0 (current)
Parsed [ 0.016s] (current)
Building datafusion-sqllogictest v55.1.0 (baseline)
Built [ 96.396s] (baseline)
Parsing datafusion-sqllogictest v55.1.0 (baseline)
Parsed [ 0.018s] (baseline)
Checking datafusion-sqllogictest v55.1.0 -> v55.1.0 (no change; assume patch)
Checked [ 0.112s] 223 checks: 223 pass, 31 skip
Summary no semver update required
Finished [ 196.784s] datafusion-sqllogictest
```
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: preserve outer join equivalences for null rows [datafusion]
Posted by "Prajwal-k-tech (via GitHub)" <gi...@apache.org>.
Prajwal-k-tech commented on PR #26042:
URL: https://github.com/apache/datafusion/pull/26042#issuecomment-5986352946
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 `action_required` before starting jobs. Could a maintainer approve them so CI can verify the updated commit?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: preserve outer join equivalences for null rows [datafusion]
Posted by "kumarUjjawal (via GitHub)" <gi...@apache.org>.
kumarUjjawal commented on code in PR #26042:
URL: https://github.com/apache/datafusion/pull/26042#discussion_r4197675545
##########
datafusion/physical-expr/src/equivalence/class.rs:
##########
@@ -847,6 +908,24 @@ impl EquivalenceGroup {
Ok(group)
}
+ /// Retain only expressions that evaluate to NULL when the joined row is
+ /// null-extended. Rebuilding the classes also drops constant information
+ /// 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| {
+ !is_volatile(expr)
+ && expr
+ .evaluate(null_batch)
+ .and_then(|value| value.into_array(1))
+ .is_ok_and(|array| array.null_count() == 1)
Review Comment:
`array.null_count() == 1` reads the physical null buffer, so arrays with only logical nulls report 0 and their valid equivalences are dropped.
##########
datafusion/physical-expr/src/equivalence/class.rs:
##########
@@ -815,17 +819,74 @@ impl EquivalenceGroup {
join_type: &JoinType,
left_size: usize,
on: &[(PhysicalExprRef, PhysicalExprRef)],
+ ) -> Result<Self> {
+ self.join_with_optional_schema(right_equivalences, join_type, left_size, None, on)
+ }
+
+ /// Combine equivalence groups for a join with its output schema available.
+ pub(crate) fn join_with_schema(
+ &self,
+ right_equivalences: &Self,
+ join_type: &JoinType,
+ left_size: usize,
+ join_schema: &SchemaRef,
+ on: &[(PhysicalExprRef, PhysicalExprRef)],
+ ) -> Result<Self> {
+ self.join_with_optional_schema(
+ right_equivalences,
+ join_type,
+ left_size,
+ Some(join_schema),
+ on,
+ )
+ }
+
+ fn join_with_optional_schema(
+ &self,
+ right_equivalences: &Self,
+ join_type: &JoinType,
+ left_size: usize,
+ join_schema: Option<&SchemaRef>,
+ on: &[(PhysicalExprRef, PhysicalExprRef)],
) -> Result<Self> {
let group = match join_type {
JoinType::Inner | JoinType::Left | JoinType::Full | JoinType::Right => {
- let mut result = Self::new(
- self.iter().cloned().chain(
- right_equivalences
- .iter()
- .map(|cls| cls.try_with_offset(left_size as _))
- .collect::<Result<Vec<_>>>()?,
- ),
+ let mut left = self.clone();
+ let mut right = Self::new(
+ right_equivalences
+ .iter()
+ .map(|cls| cls.try_with_offset(left_size as _))
+ .collect::<Result<Vec<_>>>()?,
);
+
+ // Outer joins introduce rows where one side's columns are all
+ // NULL. Keep only equivalences that remain true for that
+ // null-extended row; expressions such as coalesce(a, 0) do
+ // not, even when they were equivalent to a column before the
+ // join.
+ if let Some(join_schema) = join_schema {
+ match join_type {
+ JoinType::Left => {
+ right = right.with_null_preserving_expressions(&null_batch(
+ join_schema,
+ )?);
+ }
+ JoinType::Right => {
+ left = left.with_null_preserving_expressions(&null_batch(
+ join_schema,
+ )?);
+ }
+ JoinType::Full => {
+ let batch = null_batch(join_schema)?;
Review 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.**
##########
datafusion/physical-expr/src/equivalence/class.rs:
##########
@@ -847,6 +908,24 @@ impl EquivalenceGroup {
Ok(group)
}
+ /// Retain only expressions that evaluate to NULL when the joined row is
+ /// null-extended. Rebuilding the classes also drops constant information
+ /// 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| {
Review 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.
##########
datafusion/physical-expr/src/equivalence/class.rs:
##########
@@ -957,6 +1059,18 @@ mod tests {
use crate::expressions::{BinaryExpr, Column, binary, col, lit};
use arrow::datatypes::{DataType, Field, Schema, SchemaRef};
+ #[test]
+ fn full_join_equivalences_support_empty_schema() -> Result<()> {
+ let schema = Arc::new(Schema::empty());
+ let left = EquivalenceGroup::new(vec![]);
+ let right = EquivalenceGroup::new(vec![]);
+
+ let result = left.join_with_schema(&right, &JoinType::Full, 0, &schema, &[])?;
+
+ assert_eq!(result.iter().count(), 0);
+ Ok(())
+ }
+
use datafusion_expr::Operator;
Review 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.
##########
datafusion/physical-expr/src/equivalence/class.rs:
##########
Review Comment:
https://github.com/apache/datafusion/blob/5f1cabcd87a9964ce654186b5056f4acf06ec2cc/datafusion/physical-expr/src/equivalence/class.rs#L816
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.
##########
datafusion/physical-expr/src/equivalence/class.rs:
##########
@@ -847,6 +908,24 @@ impl EquivalenceGroup {
Ok(group)
}
+ /// Retain only expressions that evaluate to NULL when the joined row is
+ /// null-extended. Rebuilding the classes also drops constant information
+ /// 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| {
+ !is_volatile(expr)
+ && expr
+ .evaluate(null_batch)
Review 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.
##########
datafusion/sqllogictest/test_files/joins.slt:
##########
@@ -2074,6 +2074,55 @@ set datafusion.explain.logical_plan_only = false;
statement ok
set datafusion.execution.target_partitions = 4;
+statement ok
+set datafusion.execution.target_partitions = 1;
+
+statement ok
+CREATE TABLE outer_join_sort_left (id BIGINT) AS VALUES (1), (2);
+
+statement ok
+CREATE TABLE outer_join_sort_right (a BIGINT, b BIGINT) AS VALUES (2, 2), (3, 3);
+
+query III
Review 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.
##########
datafusion/physical-expr/src/equivalence/class.rs:
##########
@@ -902,6 +981,29 @@ impl EquivalenceGroup {
}
}
+/// Build a one-row batch with every field NULL so expressions can be checked
+/// for their behavior on an outer join's null-extended rows.
+fn null_batch(schema: &SchemaRef) -> Result<RecordBatch> {
+ let schema = Arc::new(Schema::new(
+ schema
+ .fields()
+ .iter()
+ .map(|field| Field::new(field.name(), field.data_type().clone(), true))
Review Comment:
`null_batch` rebuilds each field with `Field::new(name, type, true)`, which drops field metadata such as extension-type annotations.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: preserve outer join equivalences for null rows [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #26042:
URL: https://github.com/apache/datafusion/pull/26042#issuecomment-6506169840
Thanks for the contribution! I am closing this under DataFusion's AI Dump policy.
https://datafusion.apache.org/contributor-guide/ai-policy.html#what-is-an-ai-dump-and-why-it-is-not-helpful
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org