You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "zhuqi-lucas (via GitHub)" <gi...@apache.org> on 2026/08/13 09:07:08 UTC
[PR] fix: keep a projection's schema in step with its simplified expressions [datafusion]
zhuqi-lucas opened a new pull request, #24321:
URL: https://github.com/apache/datafusion/pull/24321
## Which issue does this close?
Works towards #24284. Opened as a **draft on purpose**: the change itself is small, but it enforces an invariant the tree does not currently maintain, and I would like CI to enumerate the full fallout before anyone spends review time on it. See "What this is really asking" below.
## Rationale for this change
`LogicalPlan::map_expressions` replaces a projection's expressions while keeping its existing schema, so `SimplifyExpressions` can leave the two out of step. Constant folding turns a function call, whose field the planner derived as nullable, into a non-null literal whose field is not, and the schema keeps the pre-folding answer.
Today that is invisible because `OptimizeProjections` rebuilds the projections it touches with `Projection::try_new`, deriving the schema again and normalising it back. So whether a stale schema reaches the final plan depends on which rules happen to fire.
Where it does reach the final plan it is observable, not just cosmetic. In `schema_evolution_nested.slt` the projection feeds `COPY (SELECT ...) TO ... STORED AS PARQUET`, so the stale nullability is written into the parquet file.
## What changes are included in this PR?
`SimplifyExpressions` derives the projection schema after rewriting, and only when the expressions actually changed.
## What this is really asking
Enforcing "a projection's schema equals what `projection_schema` would derive from its expressions" surfaces places where the tree does not currently hold that. Two so far:
**1. Stale nullability after constant folding.** Fixed by this PR. With it, the four `roundtrip_literal_*` tests in `datafusion-substrait` and `schema_evolution_nested.slt` pass without touching any test or snapshot, which is what makes me think the invariant is the right one.
**2. Logical and physical nullability disagree for `coalesce`.** Surfaced by this PR, not caused by it:
```sql
SELECT DISTINCT COALESCE(+cor0.col1, cor0.col1, 2, cor0.col1) FROM tab0 AS cor0 GROUP BY cor0.col1
```
```
Internal error: Physical input schema should be the same as the one converted from logical input schema. Differences:
- field nullability at index 0 [coalesce(cor0.col1,cor0.col1,Int64(2),cor0.col1)]: (physical) true vs (logical) false.
```
`coalesce`'s `return_field_from_args` is "non-null if any argument is non-null", and the literal `2` is non-null, so `false` is the accurate answer. On `main` the query runs because the projection's schema still holds the pre-simplification `true`, which happens to agree with the physical side. Making the logical side accurate is what puts the two out of step.
From `datafusion-testing/data/sqlite/random/groupby/slt_good_1.slt:33179`, so it only shows up under `--include-sqlite`.
That is the question for #24284: is the invariant something the tree should hold, in which case the logical/physical asymmetry is a separate bug worth fixing, or is a projection's schema a declared output that expression rewrites must not change, in which case `OptimizeProjections` should stop silently re-deriving it and this PR is the wrong direction.
I have no stake in which answer; I ran into this while trying to stop `OptimizeProjections` from recomputing projection schemas (#24264, #24281) and would rather settle the semantics than work around them. Happy to implement either direction.
## Are there any user-facing changes?
Where a stale schema previously reached the final plan, that plan's nullability changes to match its expressions. No test or snapshot in the tree needed updating for the suites I can run locally (`datafusion-optimizer`, `-expr`, `-common`, `-sql`, `-substrait`, and `schema_evolution_nested.slt`); the sqlite corpus is what turned up the `coalesce` case, and CI on this PR should show whether there are more.
--
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: keep a projection's schema in step with its simplified expressions [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #24321:
URL: https://github.com/apache/datafusion/pull/24321#issuecomment-5278947943
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/24321?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 `93.75000%` with `1 line` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 81.13%. Comparing base ([`c08832d`](https://app.codecov.io/gh/apache/datafusion/commit/c08832d481cea2dcea98e43393e3dd640d421064?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`5cd969b`](https://app.codecov.io/gh/apache/datafusion/commit/5cd969b51018d006dab81891b3d1c294105bbaef?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 11 commits behind head on main.
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/24321?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...timizer/src/simplify\_expressions/simplify\_exprs.rs](https://app.codecov.io/gh/apache/datafusion/pull/24321?src=pr&el=tree&filepath=datafusion%2Foptimizer%2Fsrc%2Fsimplify_expressions%2Fsimplify_exprs.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9vcHRpbWl6ZXIvc3JjL3NpbXBsaWZ5X2V4cHJlc3Npb25zL3NpbXBsaWZ5X2V4cHJzLnJz) | 93.75% | [0 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/24321?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 #24321 +/- ##
==========================================
- Coverage 81.29% 81.13% -0.16%
==========================================
Files 1110 1112 +2
Lines 385205 386726 +1521
Branches 385205 386726 +1521
==========================================
+ Hits 313145 313769 +624
- Misses 53580 54482 +902
+ Partials 18480 18475 -5
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/24321?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: keep a projection's schema in step with its simplified expressions [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #24321: fix: keep a projection's schema in step with its simplified expressions
URL: https://github.com/apache/datafusion/pull/24321
--
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: keep a projection's schema in step with its simplified expressions [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #24321:
URL: https://github.com/apache/datafusion/pull/24321#issuecomment-6136425863
Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days.
--
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