Repository navigation
[refine](column) Normalize constant expression results - #69679
Merged
Mryange merged 2 commits intoOct 29, 2026
Merged
Conversation
### What problem does this PR solve? Issue Number: N/A Related PR: apache#66614 Problem Summary: Constant expressions may return ColumnConst or an ordinary single-row column, while scan consumers assume a const wrapper. Replace ColumnPtrWrapper with ConstantExprResult, normalize the result without copying, and migrate consumers to read-only accessors. Wrap the data column only when broadcasting. ### Release note Handle constant expressions returning ordinary single-row columns consistently in scan predicate processing. ### Check List (For Author) - Test: ASAN BE UT build passed; 275 tests passed and 1 existing test skipped across 16 suites. Formatting, build hygiene, and changed-line clang-tidy checks passed; existing complexity diagnostics remain on unchanged test code. - Behavior changed: Yes. Normalize constant expression results and enforce the single-row invariant. - Does this need documentation: No
Mryange
requested review from
airborne12,
csun5285 and
eldenmoon
as code owners
October 27, 2026 08:41
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Reviewed all 43 changed files at head 9f5b362. One new P1 wrong-results finding requires changes: enabling function pushdown can make a newly accepted plain NULL LIKE pattern match empty-string rows. No existing inline findings require retention. No additional user focus was supplied.
Critical checkpoint conclusions:
- Goal and correctness: normalizing cached constants to an owned single-row datum fixes the physical ColumnConst assumption; the new LIKE admission still needs the NULL safeguard described inline. Transaction visibility, version and delete-bitmap rules are unaffected.
- Scope and abstraction: focused representation refactor; consumers consistently use ConstantExprResult and no stale ColumnPtrWrapper use remains.
- Concurrency and locks: constants initialize during fragment open before thread-context clones share them. Access is read-only; no new shared mutation, locks, lock-order change or heavy locked work was found.
- Lifecycle and initialization: intrusive column ownership survives outer-wrapper release, clones retain the cache, and pushed scan contexts retain StringRef backing storage. Broadcasting constructs a new outer ColumnConst. No new ownership cycle or cross-translation-unit static initialization dependency was found.
- Configuration: no new setting; the finding requires existing enable_function_pushdown=true. Its default is false. The source-traced JSON/Coalesce example can reach BE with the default enable_fold_constant_by_be=false.
- Compatibility: internal C++ callers are migrated; function names, SQL return types, FE/BE protocol and persisted formats are unchanged. No new platform-library dependency was found in the diff; release ELF compatibility was not executed or audited.
- Thrift/protobuf field IDs: no gensrc changes; the upgrade-ID checkpoint is not applicable.
- Parallel paths: OLAP, file and mock scan reads, scalar function opens, collection IN, ANN extraction, test utilities and broadcasts were reviewed. File-scan partial acceptance is preserved. The OLAP LIKE path is the exception reported inline.
- Conditional checks and error handling: the one-row check enforces the existing execute_column(...,1) contract; errors propagate through existing Status/exception boundaries. Plain nullable NULL LIKE patterns need an explicit disposition before storage-predicate creation.
- Test coverage: inspected ownership/pointer identity, direct and wrapped values, nullable true/false/NULL, nested array NULLs, invalid multirow inputs, zero/one/many-row broadcast, scalar arguments and ANN cases. Missing coverage is a plain nullable NULL LIKE pattern with function pushdown and its query result; clone/concurrency behavior was traced in source rather than directly tested here.
- Test results: changed expectations are consistent with the reviewed paths; no regression result artifacts were changed. The PR author's reported test runs were not independently reproduced.
- Observability: existing invariant/status errors suffice for the representation change; no additional logging or metrics requirement was identified.
- Transactions and persistence: no transaction, EditLog, replay or failover behavior is changed.
- Writes and atomicity: no data-write or commit path is changed; the reported defect affects query filtering.
- FE/BE variables: no new transmitted variable or sending path is introduced.
- Performance: normalization retains the existing row without copying; broadcast stays O(1), and scalar accessor changes add no row-level materialization. No additional performance defect was substantiated.
- Other issues and user concerns: both initial risks and all reviewer candidates have explicit dispositions; the duplicate NULL-LIKE candidate is represented by one inline finding. No additional user concern or unresolved code candidate remains.
Static source review only: no builds, tests, benchmarks or runtime SQL were executed; the inline example is inferred from the producer/consumer chain. Review converged in 2 rounds after complete coverage, independent verification, duplicate resolution and the final sweep.
Review converged after 2 round(s).
| pdt = PushDownType::UNACCEPTABLE; | ||
| return Status::OK(); | ||
| } | ||
| *constant_str = const_col_wrapper->column().get_data_at(0); |
Contributor
There was a problem hiding this comment.
[P1] Preserve NULL semantics for newly accepted plain LIKE patterns
With enable_function_pushdown=true, a constant coalesce(...) can return a plain ColumnNullable(String) containing NULL, which this path previously rejected. For example, on a duplicate-key table whose VARCHAR key k contains '', consider:
WHERE k LIKE coalesce(
jsonb_extract_string(CAST('{}' AS JSON), '$.x'),
jsonb_extract_string(CAST('{}' AS JSON), '$.y'))The missing paths yield NULL, and VectorizedCoalesceExpr caches an ordinary nullable row. This read produces {nullptr, 0}, but accepting it builds a LikeColumnPredicate whose FunctionLike::construct_like_const_state treats it as an empty pattern and selects constant_equals_fn_scalar. The accepted key predicate also removes the residual LIKE expression, so the empty-key row passes although SQL NULL must reject every row. With the default FE-only constant folding, the JSON extraction is not folded away. Preserve expression fallback for a NULL pattern, or explicitly reject all rows, before admitting it to the storage filter; add a plain-null pattern case. This example is source-traced, not executed.
### What problem does this PR solve? Issue Number: N/A Related PR: apache#69679 Problem Summary: Normalizing constant expression results makes plain nullable NULL LIKE patterns eligible for storage pushdown. The storage predicate treats a NULL StringRef as an empty pattern, which can return incorrect rows for LIKE and NOT LIKE. Reject NULL patterns before creating a storage predicate so expression evaluation preserves SQL NULL semantics. Keep non-NULL empty patterns eligible for pushdown. ### Release note Preserve NULL LIKE and NOT LIKE pattern semantics when function pushdown is enabled. ### Check List (For Author) - Test: ASAN BE UT build and OlapScanLikeConstantPatternTest.* passed (3 tests). clang-format 16, git diff --check, and BE build hygiene passed. - Behavior changed: Yes. NULL LIKE patterns retain expression evaluation instead of storage pushdown. - Does this need documentation: No
Contributor
Author
|
/review |
Contributor
TPC-H: Total hot run time: 28837 ms |
Contributor
There was a problem hiding this comment.
Static review of PR #69679 at 40dc0a19edecfa25508992c2ee1a3e98dd0dc397: no new actionable findings.
Existing P1 4387798062 is fixed at this head. OlapScanLocalState::_should_push_down_function_filter checks the normalized pattern's NULL bit before reading its bytes; rejection prevents storage-predicate creation and retains expression evaluation. The new tests cover plain/wrapped nullable patterns, empty and nonempty values, and nonempty payloads hidden by NULL. No existing blocking ID is carried forward and no duplicate comment is posted.
Critical checkpoint conclusions:
- Goal:
ConstantExprResultconsistently stores one owning data row for plain and wrapped constants, andget_result_from_constbroadcasts it for the requested batch size. The constructor, ownership, array/null, broadcast and scan tests cover these contracts in source; they were not executed. - Scope and clarity: the 44-file change is a focused producer/API/consumer migration with corresponding test and mock updates.
- Concurrency and locks: original fragment initialization populates the cache before context cloning; clones retain shared owners and initialize thread-local state. No new shared mutation, thread entry, lock order or blocking operation was introduced.
- Lifecycle and static initialization: the inner COW column remains owned after its outer wrapper is released; expression/context owners preserve borrowed string data and ANN values. No ownership cycle or new static initializer was identified.
- Configuration: no configuration item or dynamic-update behavior changes.
- Compatibility: the changed C++ interfaces are migrated together in this tree; no serialized block, storage format or wire protocol changes were found. No mixed-version decoding path is added.
- Thrift/protobuf: no
gensrc/thriftorgensrc/protochange. - Parallel paths: OLAP/file/mock scans, scalar and collection IN, string/regexp/date/JSON/dictionary functions, random seeds, ANN extraction and cached expression broadcasting use the normalized representation. No old
ColumnPtrWrapperreference remains underbe. - Conditional checks: the new NULL-pattern rejection has a concrete explanatory comment; the one-row constructor check matches the existing
execute_column(count=1)validation. Ordinary NOT LIKE remains a generic expression through the scan's slot-child gate. - Test coverage: added tests exercise nullable true/false/NULL scan completion and dependency readiness, binary NULL extraction, LIKE fallback, owner lifetime, nested NULLs and zero/one/multiple-row broadcasts. The scan tests mock constant production; full SQL/coalesce behavior and concurrent execution remain source-traced rather than executed.
- Expected results: no regression output file changes; the changed unit-test expectations agree with the traced one-row/NULL contracts.
- Observability: this representation refactor adds no distributed operation requiring new metrics or logging; invariant failures retain explicit failure behavior.
- Transactions and persistence: no EditLog, commit/replay, failover, version-visibility or delete-bitmap changes.
- Data writes and atomicity: no write path, storage mutation or new crash-recovery behavior.
- FE/BE variables: no new variable, function registration, argument/return type or sending path.
- Performance and memory: normalization shares existing owning column storage without materialization or value copies, removes repeated wrapper dispatch/unpacking, and adds no large allocation or row-loop copy. Cache ownership continues through the existing COW allocation paths.
- Errors and data correctness: scalar/Field/NULL reads preserve row-zero semantics; constant WHERE NULL/false still signals EOS and readiness; binary NULL and ANN validation retain their error/rejection paths. No distinct additional defect was verified.
Coverage: all 44 authoritative changed paths and the relevant producer/consumer/lifecycle chains were reviewed by the main reviewer, two complete-review scopes and a separate constant-boundary risk scope. The initial shape/ownership and NULL/pushdown risks were dismissed with source evidence; no unresolved candidate or static coverage gap remains. All assigned reviewers returned NO_NEW_VALUABLE_FINDINGS in round 1. Review converged after 1 round.
User focus: no additional focus was supplied. Validation was static inspection only: no builds, tests, benchmarks, formatting/link checks or runtime reproduction were run.
Review converged after 1 round(s).
Contributor
TPC-DS: Total hot run time: 151860 ms |
Contributor
ClickBench: Total hot run time: 24.58 s |
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 28366 ms |
Contributor
TPC-DS: Total hot run time: 152264 ms |
Contributor
ClickBench: Total hot run time: 24.44 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
CalvinKirs
added a commit
to CalvinKirs/incubator-doris
that referenced
this pull request
Oct 29, 2026
### What problem does this PR solve? Related PR: apache#69679, apache#69257 Problem Summary: ConstantExprResult now owns its column behind accessors, but HLL downsample still reads column_ptr as a field. This breaks the master BE compile and performance builds at both optional argument reads. Pass column() to the existing const IColumn parsing helpers. ### Release note None ### Check List (For Author) - Test: clang-format 16 passed; helper and accessor signatures checked. Full BE compilation will be verified by CI after pushing this fix. - Behavior changed: No - Does this need documentation: No
3 of 16 tasks
3 of 7 tasks
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.
What problem does this PR solve?
Issue Number: N/A
Related PR: #66614
Problem Summary:
Root cause: expression constness does not guarantee a physical
ColumnConst. Cached constant expressions can return either a const wrapper or a single-row data column, while scan consumers sometimes cast the result directly toColumnConst.Store an owned single-row data column in
ConstantExprResult, unwrapColumnConstwithout copying, and expose read-only accessors. Migrate scan, scalar-function, and ANN consumers, and recreateColumnConstwhen broadcasting the cached value. Preserve current function initialization and NULL handling.Release note
Handle constant expressions that return ordinary single-row columns consistently in scan predicate processing.
Check List (For Author)
MathFunctionTest.random_testwas skipped. Coverage includes zero-copy normalization, nullable values and ownership, arrays with nested NULLs, invalid multi-row input, broadcasting, scan predicates, function constant arguments, and ANN extraction.git diff --check, and BE build hygiene checks passed.Check List (For Reviewer who merge this PR)