[UT](be) improve if expr be UT coverage#64489
Open
jacktengg wants to merge 3 commits into
Open
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Automated review result: no blocking code issues found.
Critical checkpoint conclusions:
- Goal and proof: This PR is focused on increasing BE unit-test coverage for
FunctionIf; the new tests exercise scalar dispatch, generic/string execution, nullable then/else handling, NULL then/else branches, and nullable condition handling. The implementation change is limited to#ifdef BE_TESTwrappers used by those tests. - Scope: The change is small in behavioral surface: one test-only wrapper block in
if.cppplus one new BE unit test file. No productionif()semantics are changed. - Concurrency and lifecycle: No new shared mutable runtime state, thread entry, locking, static initialization dependency, or special lifecycle management is introduced.
- Config, compatibility, persistence, transactions: No new config items, wire/storage format changes, FE-BE protocol changes, persistence paths, or transaction/write paths are involved.
- Parallel paths and conditions: I checked the
FunctionIfcall chain throughSimpleFunctionFactory,nullif, andifnull; the test-only direct wrappers are scoped to branches that are otherwise hard to reach through public factory execution. - Error handling and memory: New wrappers preserve
Statusreturns. No new production allocation ownership or memory-tracking concern is introduced outside BE_TEST-only code. - Test coverage:
be/test/CMakeLists.txtuses recursive globbing, so the newbe/test/exprs/function/function_if_test.cppis included indoris_be_test. The tests cover the relevant nullable/const/null and scalar/generic branches added for coverage. - Observability/docs: No new logs, metrics, or documentation are needed for this test-only coverage change.
Verification notes:
- Existing inline review context was empty; no duplicate review thread issue applies.
- User focus file had no additional focus points.
git diff --checkpassed for the touched files.- Local
build-support/check-format.shcould not run because this runner does not have clang-format v16, but GitHubClang Formatteris green for this PR. - I did not run BE UT locally because this checkout has no BE build/test tree. The completed GitHub
BE UT (macOS)failure is environmental (JAVA version is 25, it must be JDK-17) and occurs before compiling this PR; Linux TeamCity BE compile/UT statuses were still queued during review.
Contributor
TPC-H: Total hot run time: 28567 ms |
Contributor
TPC-DS: Total hot run time: 167775 ms |
Issue Number: close #xxx
Related PR: #xxx
Problem Summary: 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.
None
- Test: Static check / Not run
- git diff --check -- fe/fe-core/src/main/java/org/apache/doris/nereids/rules/expression/ExpressionRuleType.java fe/fe-core/src/main/java/org/apache/doris/nereids/rules/expression/rules/NullIfToIf.java fe/fe-core/src/test/java/org/apache/doris/nereids/rules/expression/rules/NullIfToIfTest.java
- Build and tests not run per request.
- Behavior changed: No
- Does this need documentation: No
Issue Number: close #xxx
Related PR: #xxx
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.
None
- Test: Static check / Not run
- git diff --cached --check
- Build and tests not run per request.
- Behavior changed: No
- Does this need documentation: No
Issue Number: close #xxx
Related PR: #xxx
Problem Summary: NullIfToIf is a mandatory visitor-based expression rewrite rule so it is not controlled by disable_nereids_expression_rules. It was still placed inside the bottomUp pattern-rule varargs in ExpressionNormalization, which only accepts ExpressionPatternRuleFactory and causes FE compilation to fail. Split normalization into a pre-fold bottom-up pattern batch, the mandatory NullIfToIf rule, and the remaining bottom-up pattern batch.
None
- Test: Static check / Not run
- git diff --check -- fe/fe-core/src/main/java/org/apache/doris/nereids/rules/expression/ExpressionNormalization.java
- Build and tests not run per request.
- Behavior changed: No
- Does this need documentation: No
jacktengg
requested review from
924060929,
englefly,
morrySnow and
starocean999
as code owners
June 15, 2026 06:46
This was referenced Jun 15, 2026
zclllyybb
added a commit
that referenced
this pull request
Jun 15, 2026
…mpt (#64536) Problem Summary: Automated code review runs relied on the reviewer to infer which module-specific AGENTS.md files apply to a pull request. Recent Litefuse traces showed that reviews often skipped ancestor guides such as the repository root AGENTS.md, fe/AGENTS.md, and be/test/AGENTS.md even when the changed files made them applicable. This change fetches the PR changed file list, derives every existing AGENTS.md file from the changed file ancestor directories, records the list in the review context, and injects the required guide paths directly into the first Codex review prompt. We tested 5 PRs that did not read the AGENTS.md file during previous pipeline runs, and the new results are as follows: | Origin PR | Test PR | Run | Litefuse Trace | Result | |---|---:|---:|---|---| | #64478 | zclllyybb#36 | `27536249602` | `d76b77ac4e0d52d96f413b154c9f2571` | Read All | | #64458 | zclllyybb#37 | `27536258049` | `8db34388a8379acdcd9e48720f11225f` | Read All | | #64392 | zclllyybb#38 | `27536266784` | `a5e37fba03375a9517f7a67e33f70d39` | Read All | | #64489 | zclllyybb#39 | `27536275680` | `22ace18d70ece96b0ca7fa73123b62da` | Read All | | #64419 | zclllyybb#41 | `27538239601` | `1803c040e4140d61e840e4e1526190e7` | Read All |
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
2 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.
What problem does this PR solve?
Before:
After:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)