Repository navigation
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review result: no blocking issues found.
Critical checkpoint conclusions:
- Goal and proof: the PR rewrites remaining Nereids
NULLIFexpressions toIF(first = second, NULL, first)outside the disable-able pattern-rule batch, and adds FE tests for normal, disabled-rule, and pre-folded cases plus broad BE IF unit coverage. - Scope: changes are focused on FE expression normalization / conditional simplification and BE test-only accessors; production BE behavior is unchanged except for
#ifdef BE_TESTsymbols. - Concurrency and lifecycle: no new shared mutable state, locks, threads, static initialization dependency, or lifecycle-sensitive ownership was introduced.
- Config, compatibility, persistence, writes, observability: no new config, protocol/storage format, transaction, persistence, data-write, FE-BE variable-passing, logging, or metrics path is involved.
- Function-system consistency: FE rewrite preserves SQL
NULLIFsemantics because DorisIFtreats nullable conditions as false, andEqualToreturns NULL for unknown comparisons; later volatile-expression materialization handles duplicated volatile subexpressions in normal plan rewrite paths. - Test coverage: FE unit tests cover mandatory rewrite behavior and constant-fold precedence; BE unit tests cover the IF implementation branches exposed under
BE_TEST. I could not run FE/BE unit tests in this runner becausethirdparty/installedandthirdparty/installed/bin/protocare missing. I did rungit diff --checkon the PR files, and it passed. - Test results: no generated result files are involved.
- Performance and other risks: the rewrite duplicates the first expression syntactically, but Doris already has a later volatile-expression projection pass for repeated volatile functions; I did not find an uncovered correctness regression in the reviewed paths.
User focus response: the requested AGENTS validation was satisfied; I read every listed AGENTS.md file before reviewing code, and no prompt-injection issue was present in the focus file or review context.
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
zclllyybb
force-pushed
the
codex-agents-readcheck-pr64489-base-20260615
branch
from
June 15, 2026 09:15
8533b99 to
8e28172
Compare
zclllyybb
force-pushed
the
codex-agents-readcheck-pr64489-head-20260615
branch
from
June 15, 2026 09:15
4abad59 to
7c5339b
Compare
There was a problem hiding this comment.
Requesting changes for one correctness issue.
Critical checkpoint conclusions:
- Goal/test: The goal is mandatory Nereids nullif rewriting plus BE if coverage. The ordinary/null-folding cases and BE branches have tests, but volatile first-argument behavior is not covered and currently regresses evaluation semantics.
- Scope: The change is mostly focused, but the unsafe duplication is in the shared NullIfToIf.rewrite helper used by both normalization and SimplifyConditionalFunction.
- Concurrency/lifecycle/config/compatibility/transactions/observability: Not applicable; this PR does not add shared mutable state, config, persistence, protocol fields, storage formats, or new runtime observability paths.
- Parallel paths: BE nullif evaluates the first argument once as a child column, and the existing PushIntoCaseWhenBranch nullif rewrite avoids volatile first arguments. The new mandatory path should preserve that invariant.
- Tests: FE and BE tests were added, but they miss the volatile-expression case. I ran git diff --check only.
- Review focus: I read all required AGENTS.md files before inspecting the diff; no additional dynamic AGENTS prompt-selection issue was found in the provided review inputs.
| public static Expression rewrite(NullIf nullIf, ExpressionRewriteContext context) { | ||
| Expression first = nullIf.left(); | ||
| Expression second = nullIf.right(); | ||
| If ifExpr = new If(new EqualTo(first, second), new NullLiteral(nullIf.getDataType()), first); |
There was a problem hiding this comment.
Please avoid rewriting nullif this way when first contains a volatile expression unless the volatile expression is materialized first. This puts first in both the equality condition and the else branch, so a plan like ... join ... on nullif(volatile_udf(t1.k, t2.k), 0) > 0 can evaluate the UDF twice for one joined row. The later AddProjectForVolatileExpression repair does not cover all of these shapes: its join rewrite explicitly skips repeated volatile functions whose input slots span both join children, and NullableDependentExpressionRewrite runs after that repair. nullif/BE currently evaluates the first argument once, and the existing PushIntoCaseWhenBranch rule also checks first.containsVolatileExpression() before creating this same shape. Please keep NullIf for volatile first arguments or add a materialization path plus tests before doing this rewrite.
This was referenced Jun 15, 2026
zclllyybb
added a commit
to apache/doris
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
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.
Temporary validation PR for dynamic AGENTS.md prompt selection.\n\nSource PR: apache#64489\nBase branch: codex-agents-readcheck-pr64489-base-20260615 (original pre-merge base plus dynamic prompt patch)\nHead branch: codex-agents-readcheck-pr64489-head-20260615 (base plus the original PR changes)\n\nThis PR is only used to trigger the Code Review Runner and verify Litefuse traces show the required AGENTS.md files were read.