[fix](be) Normalize constant nullable TopN predicate results - #66442
Open
HappenLee wants to merge 2 commits into
Open
[fix](be) Normalize constant nullable TopN predicate results#66442HappenLee wants to merge 2 commits into
HappenLee wants to merge 2 commits into
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 conclusion: changes requested.
One blocking correctness issue is reported inline: nullable TopN normalization can produce a physically non-nullable constant Boolean while the expression remains declared nullable, and FileScannerV2's strict constant-filter execution rejects that result.
Critical checkpoint conclusions:
- Goal and proof: The change targets the real all-NULL
ColumnConst(ColumnNullable(...))failure, and the new unit test proves the intended NULLS FIRST/LAST behavior through the relaxed column/filter APIs. It does not prove the strict block-execution integration where the accepted issue occurs. - Scope and design: The two-file change is small and focused, and its recursion preserves constant cardinality and COW ownership. It is not end-to-end complete because it strips a nullable physical wrapper without keeping every caller's declared/physical type contract consistent.
- Concurrency: Runtime-predicate bound access remains protected by the existing shared mutex, null ordering is immutable, and the change adds no thread, lock, atomic, or deadlock concern.
- Lifecycle: The existing
RuntimePredicateobserver lifetime and expression prepare/open/clone lifecycle remain valid; no new static initialization or ownership cycle is introduced. - Configuration and compatibility: No configuration, serialized format, symbol, FE-BE protocol, storage, or rolling-upgrade contract changes are involved. Raw fixed/binary, dictionary, and external min-max paths retain consistent ASC/DESC and NULL ordering semantics.
- Test coverage and results: The added BE unit test has sound assertions for its path, including both NULL order modes, but misses
VExprContext::execute(Block*, int*)on a constant nullable result. A regression test for that strict API is required. This review was static as required by the runner contract; no build or test command was run here. - Observability: No new logging or metric is required for this localized expression normalization, but the current type-mismatch status would fail a valid external scan rather than preserve query semantics.
- Transactions, persistence, data writes, and transmitted variables: Not applicable; this is a read-side expression/filter change with no EditLog, transaction, storage mutation, or new FE-BE variable.
- Performance: The wrapper recursion is bounded by
ColumnConstinvariants and operates on the one-row nested payload for constants; no additional hot-path performance defect was found. - Other reviewed paths: Empty/selected batches, partial/all/no NULL inputs, bound/no-bound states, ASC/DESC, NULLS FIRST/LAST, generic filtering, raw fixed/binary evaluation, dictionary pruning, and external min-max generation were checked. No second actionable issue was substantiated.
User review focus: No additional focus was supplied, so the whole PR was reviewed.
Review completion: All candidate points were independently verified and deduplicated. The final set contains one blocking inline issue and no unresolved suspicious points.
Contributor
TPC-H: Total hot run time: 28996 ms |
Contributor
TPC-DS: Total hot run time: 166877 ms |
Contributor
ClickBench: Total hot run time: 23.79 s |
Contributor
Author
|
/review |
Contributor
|
Codex automated review failed and did not complete. Error: Review context preparation failed before Codex ran; inspect the 'Prepare authoritative PR context and required AGENTS guides' step. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
Author
|
/review |
Contributor
|
Codex automated review failed and did not complete. Error: All Codex review accounts are usage-limited; earliest retry is 2026-08-08T03:32:00Z. Please trigger /review again after that time. |
Gabriel39
approved these changes
Aug 5, 2026
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
Contributor
Author
|
run buildall |
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: Nullable TopN comparisons can return a ColumnConst that wraps a ColumnNullable result, especially when an input batch is entirely NULL. The previous normalization only handled a top-level ColumnNullable, so the constant nullable result could reach filter execution without converting SQL NULL into the NULLS FIRST or NULLS LAST filter decision. Recursively normalize constant wrappers, collapse nullable Boolean results according to the TopN null ordering, preserve the constant shape, and align the unit-test expression metadata with production nullability.
### Release note
Fix nullable TopN runtime predicate filtering for constant NULL comparison results.
### Check List (For Author)
- Test: Unit Test
- `GLIBC_COMPATIBILITY=OFF ./run-be-ut.sh -j 48 --run --filter=RuntimePredicateTest.*`
- Behavior changed: Yes, constant nullable TopN predicate results now follow NULLS FIRST/LAST semantics
- Does this need documentation: No
HappenLee
force-pushed
the
fix-topn-const-nullable-filter
branch
from
August 5, 2026 13:38
0b42c4e to
cbc9866
Compare
Contributor
Author
|
run buildall |
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.
What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: Nullable TopN comparisons can return a
ColumnConstthat wraps aColumnNullableresult, especially when an input batch is entirely NULL. The previous normalization only handled a top-levelColumnNullable, so the constant nullable result could reach filter execution without converting SQL NULL into the NULLS FIRST or NULLS LAST filter decision.This change recursively normalizes constant wrappers, collapses nullable Boolean results according to the TopN null ordering, preserves the constant shape, and aligns the unit-test expression metadata with production nullability. The new unit test covers both NULLS FIRST and NULLS LAST through
execute_columnandexecute_filter.Release note
Fix nullable TopN runtime predicate filtering for constant NULL comparison results.
Check List (For Author)
GLIBC_COMPATIBILITY=OFF ./run-be-ut.sh -j 48 --run --filter=RuntimePredicateTest.*build-support/run-clang-tidy.sh --base apache/master --build-dir be/ut_build_ASAN