Conversation
… a nullable condition
### What problem does this PR solve?
Issue Number: None
Problem Summary:
When the condition of `IF` is a `Nullable(Boolean)` column, both
`VectorizedIfExpr` and `FunctionIf` normalize it by treating NULL as false.
They did this by writing `nested[i] &= !null_map[i]` directly into the
nested column of the nullable condition.
That nested column can be shared with other columns of the same block.
`NULLIF(b, p)` is implemented as `if(b = p, NULL, b)` and wraps `b` itself
as the nested column of its `Nullable(Boolean)` result, so
`IF(NULLIF(b, p), f, b)` overwrote `b` in place: the else branch read the
polluted values and returned a wrong result, and any other projection of
`b` in the same block was polluted as well. With
`short_circuit_evaluation=true` the expression goes through a different
path and returned the correct result, which hid the bug.
Build a fresh condition column from `nested & !null_map` instead of
mutating the shared nested column in place.
### Release note
None
### Check List (For Author)
- Test:
- Unit Test: `VConditionExprIfTest.NullableCondition_NotPolluteSharedNestedColumn`, `FunctionIfTest.NullableConditionNotPolluteSharedNestedColumn` (both fail without the fix)
- Regression test: `test_if_nullable_condition`
- Behavior changed: No
- Does this need documentation: No
Claude-Session: https://claude.ai/code/session_013PjVxV5VYPRduC2fubqCzD
Member
Author
|
/review |
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
![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 summary — no actionable findings
Reviewed PR #67931 at exact head edbca7c7950eed388e92c35683e0909d3a6f49e5 against base df36e174b99557a004bc3ad57faa019da4a7d91f. The review is summary-only because no distinct inline issue remains after duplicate fencing, candidate adjudication, and two convergence rounds.
Critical checkpoint conclusions:
- Goal and data correctness: The change prevents nullable IF normalization from mutating a shared nested Boolean column. The fresh row-aligned condition implements
nested AND NOT null, preserves SQL NULL-as-false behavior, and fixes the production-reachableIF(NULLIF(b, p), f, b)alias path. - Scope and clarity: The production change is focused in the two non-short-circuit IF implementations, with targeted unit and regression coverage. No unrelated source behavior is changed.
- Concurrency: Expression blocks and temporary columns are evaluation-local. The new implementation removes the shared mutation and introduces no locks, atomics, thread entry points, or cross-thread lifecycle.
- Lifecycle and static initialization: The detached condition is owned by a local evaluation block and released with it. Recursive dispatch appends at most once because the replacement condition is non-nullable. No static initialization or ownership cycle is introduced.
- Configuration: No configuration item is added or changed. The regression suite only switches the existing short-circuit setting to cover both execution modes.
- Compatibility: No symbol, storage format, serialization, RPC, FE-BE protocol, or rolling-upgrade contract changes.
- Parallel paths:
FunctionIfandVectorizedIfExprare both corrected.ShortCircuitIfExprwas traced separately and already routes NULL through the false selector without mutating the condition. - Conditional and error handling: Top-level constants are materialized before nullable dispatch; all-NULL, nullable then/else, scalar, and generic paths remain consistent. Production Status propagation is unchanged and checked.
- Tests and results: The new FunctionIf and VectorizedIfExpr unit tests directly retain a shared nested owner and assert both the selected result and source preservation. The ordered regression queries cover short-circuit off/on and projected-source preservation; the checked
.outrows match the SQL semantics. Test discovery is automatic through the BE test glob, and no source.cppinclusion or unity-skip update is involved. - Observability: No new distributed or failure-prone operation is introduced, so additional logging or metrics are not needed.
- Transactions, persistence, and writes: Not applicable; there is no transaction, EditLog, metadata persistence, storage write, or crash-recovery change.
- FE-BE variables: No new transmitted variable or FE/BE type contract is introduced.
- Performance and memory: The fresh byte column uses the tracked Doris column allocator and has bounded local lifetime. A possible conditional-reuse idea was investigated and dismissed as a current-patch issue because nested uniqueness does not prove whole-tree COW exclusivity in either actual caller ownership graph.
- Additional review: Header hygiene, test determinism, null/const shapes, recursive termination, block reallocation, and result aliasing were checked; no further issue was substantiated.
User focus: no additional focus was provided; the complete PR was reviewed.
Verification scope: static review only, as required by the review runner contract. No build or test was executed independently. At submission time the available formatter, checkstyle, license, secrets, title, and changed-file checks were passing, while broad build/test jobs were skipped.
Member
Author
Local pipeline review — ✅ PASSschema: doris-repo-review/v1
status: PASS
pr: apache/doris#67931
commit: edbca7c7950eed388e92c35683e0909d3a6f49e5
base: df36e174b99557a004bc3ad57faa019da4a7d91f
reviewed_at: 2026-09-14T14:42+08:00
reviewer: mrhhsg
model: gpt-6-astra
effort: xhigh
findings: {blocker: 0, major: 0, minor: 0, nit: 0}
rounds: 1
converged: trueNotes for maintainers
Reviewed locally with the |
Member
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 16920 ms |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-DS: Total hot run time: 82944 ms |
Contributor
ClickBench: Total hot run time: 14.7 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
mrhhsg
marked this pull request as ready for review
September 15, 2026 02:14
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?
Issue Number: None
Problem Summary:
When the condition of
IFis aNullable(Boolean)column, bothVectorizedIfExprandFunctionIfnormalize it by treating NULL as false.They did this by writing
nested[i] &= !null_map[i]directly into thenested column of the nullable condition.
That nested column can be shared with other columns of the same block.
NULLIF(b, p)is implemented asif(b = p, NULL, b)and wrapsbitselfas the nested column of its
Nullable(Boolean)result, soIF(NULLIF(b, p), f, b)overwrotebin place: the else branch read thepolluted values and returned a wrong result, and any other projection of
bin the same block was polluted as well. Withshort_circuit_evaluation=truethe expression goes through a differentpath and returned the correct result, which hid the bug.
Build a fresh condition column from
nested & !null_mapinstead ofmutating the shared nested column in place.
Release note
None
Check List (For Author)
VConditionExprIfTest.NullableCondition_NotPolluteSharedNestedColumn,FunctionIfTest.NullableConditionNotPolluteSharedNestedColumn(both fail without the fix)test_if_nullable_conditionhttps://claude.ai/code/session_013PjVxV5VYPRduC2fubqCzD