[fix](fe) Preserve coalesce nullability after conditional rewrite#62094
Closed
starocean999 wants to merge 2 commits intoapache:masterfrom
Closed
[fix](fe) Preserve coalesce nullability after conditional rewrite#62094starocean999 wants to merge 2 commits intoapache:masterfrom
starocean999 wants to merge 2 commits intoapache:masterfrom
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
I found 1 issue that should be addressed before merge.
Critical checkpoint conclusions:
- Goal and correctness: Partially met. The PR fixes the nullability loss in
SimplifyConditionalFunction, but the same regression remains reachable through another rewrite path (FoldConstantRuleOnFE.visitNvl). - Scope: The code change is small and focused.
- Concurrency: Not applicable; this change is in FE expression rewriting only.
- Lifecycle/static init: No issue found.
- Config changes: None.
- Compatibility: No FE/BE protocol or storage compatibility issue found.
- Parallel code paths: Not fully covered.
ifnull/nvlis simplified both here and in FE constant folding, and only one path was updated. - Special conditional checks: The new
NonNullable(...)guard is reasonable for this rule. - Test coverage: Improved for the simplifier path and regression SQL, but still missing coverage for the FE constant-folding path.
- Observability: No additional observability needed.
- Transaction/persistence/data write/FE-BE variable passing: Not applicable.
- Performance: No material issue found.
- Other issues: The remaining
FoldConstantRuleOnFE.visitNvlgap can still produce incorrect nullable metadata after type coercion.
Because of that missed parallel path, I do not consider the fix complete yet.
| Nvl nvl = ctx.expr; | ||
| if (nvl.child(0) instanceof NullLiteral) { | ||
| return TypeCoercionUtils.ensureSameResultType(nvl, nvl.child(1), ctx.rewriteContext); | ||
| return keepConditionalNullability( |
Contributor
There was a problem hiding this comment.
This fixes the SimplifyConditionalFunction path, but ifnull/nvl can still lose non-nullability through FoldConstantRuleOnFE.visitNvl(). That method still rewrites nvl(nonNullableExpr, nullLiteral) / nvl(nullLiteral, nonNullableExpr) via TypeCoercionUtils.ensureSameResultType(...) and, for cases like datetime(0) -> datetime(6), it returns a nullable Cast. Since FE constant folding is invoked directly in other planner flows (FoldConstantRuleOnFE.evaluate(...), InsertIntoValuesAnalyzer, etc.), the regression is still reachable even after this change. Please apply the same nullability preservation there and add a fold-constant test for that path.
TPC-H: Total hot run time: 28900 ms |
TPC-DS: Total hot run time: 178724 ms |
Contributor
Author
|
run buildall |
TPC-H: Total hot run time: 28738 ms |
TPC-DS: Total hot run time: 178539 ms |
Contributor
FE UT Coverage ReportIncrement line coverage |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem Summary:
This PR fixes an incorrect nullability regression in Nereids when simplifying conditional functions such as
coalesceandnvl.Root cause:
SimplifyConditionalFunctionrewrites expressions likecoalesce(non_nullable_expr, ...)to the first guaranteed non-null child.ensureSameResultTypemay wrap that child with aCast.coalesceresult is guaranteed to be non-null.coalesce, and downstream plan output metadata became inconsistent.Fix:
SimplifyConditionalFunctioninstead of relying on a later nullable-adjustment pass.coalesceornvl, if the original expression is non-nullable but the rewritten expression becomes nullable because of type coercion, wrap the rewritten result withNonNullable(...).None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)