You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "blinding-pixels (via GitHub)" <gi...@apache.org> on 2026/08/11 03:25:30 UTC
[PR] Fix physical distinctness expression nullability [datafusion]
blinding-pixels opened a new pull request, #24244:
URL: https://github.com/apache/datafusion/pull/24244
## Which issue does this PR close?
- Closes #24096
## Rationale for this change
When aggregate CSE extracts a repeated truth check, the logical and physical plans assign different nullability metadata to it, causing planning to fail. Since these truth checks always return either true or false, the physical plan should mark them as non-nullable.
## What changes are included in this PR?
This PR updates physical `BinaryExpr` nullability so `IS DISTINCT FROM` and `IS NOT DISTINCT FROM` are always reported as non-nullable.
It also adds a focused unit-test matrix and a SQL logic regression covering the aggregate CSE path reported in the issue.
## Are these changes tested?
Yes. The tests cover both distinctness operators, ordinary equality as a control, and the reported aggregate query.
Formatting, Clippy, focused tests, and the extended workspace test suite all pass.
## Are there any user-facing changes?
Yes. Affected queries using repeated truth checks on nullable Boolean values now plan and execute instead of failing with a logical/physical schema mismatch.
There are no public 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 physical distinctness expression nullability [datafusion]
Posted by "blinding-pixels (via GitHub)" <gi...@apache.org>.
blinding-pixels commented on PR #24244:
URL: https://github.com/apache/datafusion/pull/24244#issuecomment-5378400720
done!
--
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 physical distinctness expression nullability [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on code in PR #24244:
URL: https://github.com/apache/datafusion/pull/24244#discussion_r3801031960
##########
datafusion/sqllogictest/test_files/scalar.slt:
##########
@@ -1853,6 +1853,24 @@ true
false
true
+# Aggregate CSE preserves the non-nullability of truth predicates
+# issue: https://github.com/apache/datafusion/issues/24096
+query IIIII
+SELECT
+ SUM(CASE WHEN b IS TRUE THEN 1 ELSE 0 END),
+ COUNT(CASE WHEN b IS TRUE THEN 1 END),
+ SUM(CASE WHEN b IS FALSE THEN 1 ELSE 0 END),
+ SUM(CASE WHEN b IS NOT TRUE THEN 1 ELSE 0 END),
+ SUM(CASE WHEN b IS NOT FALSE THEN 1 ELSE 0 END)
Review Comment:
One small coverage suggestion: it would be useful to include `IS UNKNOWN` and `IS NOT UNKNOWN` in this aggregate-CSE query too. Their physical planning lowers through the same distinctness operators using a NULL literal, so this would cover the two remaining truth-predicate lowering paths.
##########
datafusion/physical-expr/src/expressions/binary.rs:
##########
@@ -528,7 +528,13 @@ impl PhysicalExpr for BinaryExpr {
}
fn nullable(&self, input_schema: &Schema) -> Result<bool> {
- Ok(self.left.nullable(input_schema)? || self.right.nullable(input_schema)?)
+ match self.op {
+ Operator::IsDistinctFrom | Operator::IsNotDistinctFrom => Ok(false),
Review Comment:
I think we also need to make this change in the logical expression layer. `Expr::BinaryExpr` still derives nullability from its operands in `datafusion/expr/src/expr_schema.rs` (`380-381` and `528-543`), even though `IS DISTINCT FROM` and `IS NOT DISTINCT FROM` never return NULL.
With the current change, a direct distinctness expression can therefore have `nullable=true` logically but `nullable=false` physically. Aggregate CSE can extract that expression and recreate the same logical/physical schema mismatch.
Could we make the distinctness exception canonical in the logical `nullable` and `to_field` paths as well, and add regression coverage using nullable operands?
--
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 physical distinctness expression nullability [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #24244:
URL: https://github.com/apache/datafusion/pull/24244#issuecomment-5248786273
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/24244?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 `78.94737%` with `4 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 80.99%. Comparing base ([`a9b61ab`](https://app.codecov.io/gh/apache/datafusion/commit/a9b61abf04b59ff08e7254b598b07f4fa058c5d1?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`fac096a`](https://app.codecov.io/gh/apache/datafusion/commit/fac096aaec50e41d654c5b695e1072f6514f40be?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/24244?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/expressions/binary.rs](https://app.codecov.io/gh/apache/datafusion/pull/24244?src=pr&el=tree&filepath=datafusion%2Fphysical-expr%2Fsrc%2Fexpressions%2Fbinary.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9waHlzaWNhbC1leHByL3NyYy9leHByZXNzaW9ucy9iaW5hcnkucnM=) | 78.94% | [0 Missing and 4 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/24244?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 #24244 +/- ##
==========================================
- Coverage 80.99% 80.99% -0.01%
==========================================
Files 1106 1106
Lines 383549 383567 +18
Branches 383549 383567 +18
==========================================
+ Hits 310644 310654 +10
Misses 54570 54570
- Partials 18335 18343 +8
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/24244?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 physical distinctness expression nullability [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on PR #24244:
URL: https://github.com/apache/datafusion/pull/24244#issuecomment-5395791285
Thanks @blinding-pixels @kosiew LGTM
--
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 physical distinctness expression nullability [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on PR #24244:
URL: https://github.com/apache/datafusion/pull/24244#issuecomment-5378262882
@blinding-pixels
Can you resolve the merge conflicts?
--
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 physical distinctness expression nullability [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 merged PR #24244:
URL: https://github.com/apache/datafusion/pull/24244
--
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 physical distinctness expression nullability [datafusion]
Posted by "blinding-pixels (via GitHub)" <gi...@apache.org>.
blinding-pixels commented on code in PR #24244:
URL: https://github.com/apache/datafusion/pull/24244#discussion_r3821130225
##########
datafusion/physical-expr/src/expressions/binary.rs:
##########
@@ -528,7 +528,13 @@ impl PhysicalExpr for BinaryExpr {
}
fn nullable(&self, input_schema: &Schema) -> Result<bool> {
- Ok(self.left.nullable(input_schema)? || self.right.nullable(input_schema)?)
+ match self.op {
+ Operator::IsDistinctFrom | Operator::IsNotDistinctFrom => Ok(false),
Review Comment:
Thanks for that great catch. Let me know if this works better.
--
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