You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "fornwall (via GitHub)" <gi...@apache.org> on 2026/08/20 19:28:36 UTC
[PR] fix: preserve nullability for non-empty scalar subqueries [datafusion]
fornwall opened a new pull request, #24534:
URL: https://github.com/apache/datafusion/pull/24534
## Which issue does this PR close?
- Follow-up to apache/datafusion#24516, which fixed apache/datafusion#24513. The follow-up was suggested during review of that PR.
## Rationale for this change
apache/datafusion#24516 conservatively made every scalar subquery nullable because an empty result evaluates to `NULL`. Plans guaranteed to return at least one row, such as ungrouped aggregates, cannot produce that empty-result `NULL`; marking them nullable loses schema precision and prevents valid expression simplification.
## What changes are included in this PR?
- Adds a conservative `LogicalPlan::min_rows()` lower bound.
- Uses the lower bound in logical schema derivation. Physical scalar-subquery planning now defers to `Expr::nullable` instead of hardcoding `nullable = true`, so the logical rule is the single source of truth.
- Makes `max_rows()` account for the one-row form of `EmptyRelation`, keeping both bounds consistent.
## Are these changes tested?
Yes. Unit tests cover row bounds, logical schema derivation, optimizer simplification, and physical expression nullability for both possibly empty and guaranteed non-empty scalar subqueries. A sqllogictest pins the user-visible effect: `EXPLAIN` shows that `(SELECT count(*) FROM empty_table) IS NULL` now folds to `false`, eliminating the subquery from the plan. (Query results alone cannot show the change: a guaranteed non-empty scalar subquery never evaluates to `NULL` at runtime.)
As an ablation check, restoring the conservative always-nullable behavior makes the logical, optimizer, physical, and sqllogictest tests fail.
The required all-feature Clippy check and extended workspace test suite pass.
## Are there any user-facing changes?
Yes. Scalar subqueries proven to return at least one row now preserve the nullability of their projected field, allowing valid simplification. Potentially empty scalar subqueries remain nullable. `LogicalPlan::min_rows()` is a new public API.
---
AI usage: Implemented with Codex; reviewed and test coverage extended with Claude Code. I reviewed the code and made modifications where appropriate.
--
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 nullability for non-empty scalar subqueries [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #24534:
URL: https://github.com/apache/datafusion/pull/24534#issuecomment-5360947022
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/24534?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 `73.33333%` with `36 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 81.31%. Comparing base ([`dbdc627`](https://app.codecov.io/gh/apache/datafusion/commit/dbdc627deb98778dbf23cf7ccc2df7c66e1d9f55?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`7a52611`](https://app.codecov.io/gh/apache/datafusion/commit/7a52611549132481c0fbc8ca66f2eedcf8060fc2?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 10 commits behind head on main.
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/24534?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/expr/src/logical\_plan/plan.rs](https://app.codecov.io/gh/apache/datafusion/pull/24534?src=pr&el=tree&filepath=datafusion%2Fexpr%2Fsrc%2Flogical_plan%2Fplan.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9leHByL3NyYy9sb2dpY2FsX3BsYW4vcGxhbi5ycw==) | 60.75% | [16 Missing and 15 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/24534?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [datafusion/physical-expr/src/planner.rs](https://app.codecov.io/gh/apache/datafusion/pull/24534?src=pr&el=tree&filepath=datafusion%2Fphysical-expr%2Fsrc%2Fplanner.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9waHlzaWNhbC1leHByL3NyYy9wbGFubmVyLnJz) | 76.19% | [2 Missing and 3 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/24534?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 #24534 +/- ##
========================================
Coverage 81.31% 81.31%
========================================
Files 1117 1117
Lines 395911 396284 +373
Branches 395911 396284 +373
========================================
+ Hits 321918 322228 +310
- Misses 55177 55210 +33
- Partials 18816 18846 +30
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/24534?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 nullability for non-empty scalar subqueries [datafusion]
Posted by "fornwall (via GitHub)" <gi...@apache.org>.
fornwall commented on code in PR #24534:
URL: https://github.com/apache/datafusion/pull/24534#discussion_r3826643237
##########
datafusion/expr/src/expr_schema.rs:
##########
@@ -369,9 +369,8 @@ impl ExprSchemable for Expr {
Ok(expr_nullable | subquery_nullable)
}
- // A scalar subquery may return no rows, in which case it evaluates to NULL
- // regardless of the nullability of its projected field.
- Expr::ScalarSubquery(_) => Ok(true),
+ Expr::ScalarSubquery(subquery) => Ok(subquery.subquery.min_rows() == 0
+ || subquery.subquery.schema().field(0).is_nullable()),
Review Comment:
Pushed 8c1988b4c3a771e882802bcbb21ba0455f89c43e - what do you think?
--
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 nullability for non-empty scalar subqueries [datafusion]
Posted by "neilconway (via GitHub)" <gi...@apache.org>.
neilconway commented on code in PR #24534:
URL: https://github.com/apache/datafusion/pull/24534#discussion_r3826582377
##########
datafusion/physical-expr/src/planner.rs:
##########
@@ -538,8 +538,9 @@ pub fn create_physical_expr(
let dt = schema.field(0).data_type().clone();
Ok(Arc::new(ScalarSubqueryExpr::new(
dt,
- // A scalar subquery may return no rows and evaluate to NULL.
- true,
+ // Defer to the logical nullability rule, which accounts
+ // for subqueries guaranteed to produce at least one row.
Review Comment:
nit: I found this comment to be confusingly worded; I'd probably just remove it.
##########
datafusion/expr/src/expr_schema.rs:
##########
@@ -369,9 +369,8 @@ impl ExprSchemable for Expr {
Ok(expr_nullable | subquery_nullable)
}
- // A scalar subquery may return no rows, in which case it evaluates to NULL
- // regardless of the nullability of its projected field.
- Expr::ScalarSubquery(_) => Ok(true),
+ Expr::ScalarSubquery(subquery) => Ok(subquery.subquery.min_rows() == 0
+ || subquery.subquery.schema().field(0).is_nullable()),
Review Comment:
nit: We have the same code (albeit the operands are in a different order) below (~line 520). Might be worth refactoring into a helper.
--
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 nullability for non-empty scalar subqueries [datafusion]
Posted by "fornwall (via GitHub)" <gi...@apache.org>.
fornwall commented on code in PR #24534:
URL: https://github.com/apache/datafusion/pull/24534#discussion_r3826613470
##########
datafusion/physical-expr/src/planner.rs:
##########
@@ -538,8 +538,9 @@ pub fn create_physical_expr(
let dt = schema.field(0).data_type().clone();
Ok(Arc::new(ScalarSubqueryExpr::new(
dt,
- // A scalar subquery may return no rows and evaluate to NULL.
- true,
+ // Defer to the logical nullability rule, which accounts
+ // for subqueries guaranteed to produce at least one row.
Review Comment:
Agreed, deleted the comment in e2654b05f26fd190bc2cffa26158b5f5f2ab150c.
--
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 nullability for non-empty scalar subqueries [datafusion]
Posted by "neilconway (via GitHub)" <gi...@apache.org>.
neilconway commented on code in PR #24534:
URL: https://github.com/apache/datafusion/pull/24534#discussion_r3826696130
##########
datafusion/expr/src/expr_schema.rs:
##########
@@ -369,9 +369,8 @@ impl ExprSchemable for Expr {
Ok(expr_nullable | subquery_nullable)
}
- // A scalar subquery may return no rows, in which case it evaluates to NULL
- // regardless of the nullability of its projected field.
- Expr::ScalarSubquery(_) => Ok(true),
+ Expr::ScalarSubquery(subquery) => Ok(subquery.subquery.min_rows() == 0
+ || subquery.subquery.schema().field(0).is_nullable()),
Review Comment:
I like it!
--
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 nullability for non-empty scalar subqueries [datafusion]
Posted by "Dandandan (via GitHub)" <gi...@apache.org>.
Dandandan merged PR #24534:
URL: https://github.com/apache/datafusion/pull/24534
--
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