[improvement](fe) Rewrite nullif to if expression#64531
Open
jacktengg wants to merge 1 commit into
Open
Conversation
Problem Summary: `FunctionIf` in BE is only reached through `nullif` because SQL `if` is rewritten to `VectorizedIfExpr`. The `nullif` semantics can be represented as `if(first = second, null, first)`, so this adds a Nereids expression normalization rule to rewrite remaining `NullIf` expressions to `If` after existing constant folding. The nullable-dependent conditional simplification also falls back to the same rewrite so NullIf does not survive that path. Rewriting `nullif` to `if` must not be controlled by `disable_nereids_expression_rules`, otherwise users could disable the rewrite and still require BE `FunctionIf` support for `nullif`. This changes `NullIfToIf` from a pattern rule with an `ExpressionRuleType` bit into a direct visitor-based expression rewrite rule, and adds coverage that the rewrite still runs when all expression rule bits are disabled.
jacktengg
requested review from
924060929,
englefly,
morrySnow and
starocean999
as code owners
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
morrySnow
reviewed
Jun 15, 2026
| FoldConstantRule.INSTANCE, | ||
| FoldConstantRule.INSTANCE | ||
| ), | ||
| NullIfToIf.INSTANCE, |
Contributor
There was a problem hiding this comment.
如果要做这个改写，我建议在plan builder的时候就搞。这里一直想这么改，还没来得及动
![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 one blocking compatibility issue: the BE nullif implementation is removed, but the new rewrite is only guaranteed on Nereids expression-normalization paths. Legacy planner and load-expression translation can still serialize nullif as a regular FUNCTION_CALL, which will no longer resolve in BE.
Critical checkpoint conclusions:
- Correctness: blocking issue found for non-Nereids / legacy-translated
nullifexecution. - Compatibility: sessions using the original planner and load column mappings that contain
nullifcan break after this change. - Tests: the new Nereids expression tests cover the rewrite itself, but coverage is missing for
enable_nereids_planner=falseand load column mappings usingnullifafter the BE function is removed. - Style/build hygiene:
git diff --checkon the actual PR files passes. I did not run the full Doris build or test suites. - Security/threat-model: this was not requested as a security review; I did not identify a security-specific finding.
User focus: .code-review.yO7zBe/review_focus.txt had no additional user-provided focus points, so there is no focus-specific issue to report.
| register_function_string(instance); | ||
| register_function_in(instance); | ||
| register_function_collection_in(instance); | ||
| register_function_if(instance); |
Contributor
There was a problem hiding this comment.
Removing nullif from SimpleFunctionFactory is not safe unless every FE path rewrites it before thrift serialization. The new rewrite is only in Nereids expression normalization, but legacy FunctionCallExpr still serializes non-aggregate calls as TExprNodeType.FUNCTION_CALL (ExprToThriftVisitor), BE vexpr.cpp only special-cases if/ifnull/nvl/coalesce, and VectorizedFnCall now cannot resolve nullif. Non-Nereids queries (enable_nereids_planner=false) and load column mappings translated through PlanUtils.translateToLegacyExpr can still reach BE as nullif, so they will fail with "Could not find function nullif". Please keep the BE function or add the rewrite to all legacy/load translation paths and cover those paths with tests.
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 29140 ms |
Contributor
TPC-DS: Total hot run time: 168474 ms |
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
3 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:
FunctionIfin BE is only reached throughnullifbecause SQLifis rewritten toVectorizedIfExpr. Thenullifsemantics can be represented asif(first = second, null, first), so this adds a Nereids expression normalization rule to rewrite remainingNullIfexpressions toIfafter existing constant folding. The nullable-dependent conditional simplification also falls back to the same rewrite so NullIf does not survive that path.Rewriting
nulliftoifmust not be controlled bydisable_nereids_expression_rules, otherwise users could disable the rewrite and still require BEFunctionIfsupport fornullif. This changesNullIfToIffrom a pattern rule with anExpressionRuleTypebit into a direct visitor-based expression rewrite rule, and adds coverage that the rewrite still runs when all expression rule bits are disabled.Alse delete stale file
be/src/exprs/function/function_ifnull.hwhich is deleted by #58125 but accidentally add back by #58031What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)