You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "1fanwang (via GitHub)" <gi...@apache.org> on 2026/09/24 05:38:48 UTC
[PR] fix: plan aggregates over CASE conditions with multiple columns [datafusion]
1fanwang opened a new pull request, #25684:
URL: https://github.com/apache/datafusion/pull/25684
## Which issue does this PR close?
Closes https://github.com/apache/datafusion/issues/25369
## Rationale for this change
Aggregates over a repeated CASE expression can fail during planning when its condition refers to multiple columns. The reported SUM query now returns 1 instead of an internal schema error.
## What changes are included in this PR?
Physical CASE nullability uses the existing three-valued bounds for AND, OR and NOT when a condition cannot be evaluated without input rows. The aggregate's schema check stays unchanged.
The non-nullability deduction relies on NULL AND x never being true, for any x, including NULL. OR and NOT must preserve the distinction between false and NULL; the regression tests cover both nullable and non-nullable branches.
## What is the testing strategy for this PR?
The SQL regression exercises SUM, COUNT, expanded BETWEEN, NULL inputs, nullable or missing ELSE, and empty input. Parameterized unit tests cover both operand orders.
On macOS arm64 with Rust 1.98.1 and the pinned test data:
```sh
cargo test --locked --profile ci -p datafusion-sqllogictest --test sqllogictests -- cse.slt
```
<details><summary>Raw logs</summary>
Before:
```text
Internal error: Physical input schema should be the same as the one converted from logical input schema. Differences:
- field nullability at index 0 [__common_expr_1]: (physical) true vs (logical) false.
Error: Execution("1 failures")
```
After:
```text
Progress: 1/1 files completed (100%)
slt_exit_code=0
```
</details>
The physical-expression, optimizer and logical-expression crate tests and the CASE/CSE/aggregate SQL files pass. Formatting and Clippy pass with the Parquet encryption feature enabled, as in workspace CI.
## Are there any user-facing changes?
The affected aggregate queries plan and run. There is no public API change.
--
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: plan aggregates over CASE conditions with multiple columns [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #25684:
URL: https://github.com/apache/datafusion/pull/25684#issuecomment-5808666357
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/25684?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 `76.47059%` with `4 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 82.48%. Comparing base ([`5b8fcf1`](https://app.codecov.io/gh/apache/datafusion/commit/5b8fcf1b8f8b931b2e25730ffdffa3a4bc585bfe?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`4c747fa`](https://app.codecov.io/gh/apache/datafusion/commit/4c747fa28b3af4caee8a1f0dc6674ea09898bc27?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 1 commits behind head on main.
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/25684?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/case.rs](https://app.codecov.io/gh/apache/datafusion/pull/25684?src=pr&el=tree&filepath=datafusion%2Fphysical-expr%2Fsrc%2Fexpressions%2Fcase.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9waHlzaWNhbC1leHByL3NyYy9leHByZXNzaW9ucy9jYXNlLnJz) | 76.47% | [0 Missing and 4 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/25684?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 #25684 +/- ##
==========================================
- Coverage 82.48% 82.48% -0.01%
==========================================
Files 1140 1140
Lines 437614 437621 +7
Branches 437614 437621 +7
==========================================
- Hits 360983 360982 -1
- Misses 54838 54843 +5
- Partials 21793 21796 +3
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/25684?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: plan aggregates over CASE conditions with multiple columns [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on code in PR #25684:
URL: https://github.com/apache/datafusion/pull/25684#discussion_r4267779965
##########
datafusion/physical-expr/src/expressions/case.rs:
##########
@@ -2796,6 +2797,88 @@ mod tests {
case_expression_nullability(false)
}
+ #[rstest::rstest]
+ #[case::and(Operator::And, false, false)]
+ #[case::or(Operator::Or, false, true)]
+ #[case::not_and(Operator::And, true, true)]
+ #[case::not_or(Operator::Or, true, false)]
+ fn test_case_expression_nullability_with_independent_columns(
Review Comment:
Could we also test mixed AND/OR recursion with `CASE WHEN a > 1 AND (b < 3 OR b IS NULL) THEN a ELSE 0 END` and its negated form? The first should be non-nullable, while the second is nullable because `a = NULL, b = 3` makes the negated condition TRUE. This would cover nested operators beyond the current tests. Not a blocker.
--
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: plan aggregates over CASE conditions with multiple columns [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on PR #25684:
URL: https://github.com/apache/datafusion/pull/25684#issuecomment-6224453332
Thanks @1fanwang !
--
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: plan aggregates over CASE conditions with multiple columns [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb merged PR #25684:
URL: https://github.com/apache/datafusion/pull/25684
--
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: plan aggregates over CASE conditions with multiple columns [datafusion]
Posted by "1fanwang (via GitHub)" <gi...@apache.org>.
1fanwang commented on code in PR #25684:
URL: https://github.com/apache/datafusion/pull/25684#discussion_r4283758757
##########
datafusion/physical-expr/src/expressions/case.rs:
##########
@@ -2796,6 +2797,88 @@ mod tests {
case_expression_nullability(false)
}
+ #[rstest::rstest]
+ #[case::and(Operator::And, false, false)]
+ #[case::or(Operator::Or, false, true)]
+ #[case::not_and(Operator::And, true, true)]
+ #[case::not_or(Operator::Or, true, false)]
+ fn test_case_expression_nullability_with_independent_columns(
Review Comment:
Done in https://github.com/apache/datafusion/commit/743b1a918900fd912b76936b27c40afdae7f216c. Both forms are covered.
--
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: plan aggregates over CASE conditions with multiple columns [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on code in PR #25684:
URL: https://github.com/apache/datafusion/pull/25684#discussion_r4299219941
##########
datafusion/physical-expr/src/expressions/case.rs:
##########
@@ -1531,15 +1524,24 @@ fn evaluate_predicate(predicate: &Arc<dyn PhysicalExpr>) -> Result<Option<bool>>
// Evaluate the predicate and interpret the result as a boolean
let result = match predicate.evaluate(&batch) {
- // An error during evaluation means we couldn't const evaluate the predicate, so return `None`
- Err(_) => None,
- Ok(ColumnarValue::Array(array)) => Some(
+ Err(_) => {
+ if let Some(expr) = predicate.downcast_ref::<BinaryExpr>()
Review Comment:
FYI: on error we evaluate each child again, and each child can fail and recurse again. For a long `AND`/`OR` chain this is O(n^2) evaluations at planning time. Could you take a look and see if this is worthy of a followup?
--
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