[refactor](be) Encapsulate cached constant columns - #66614
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
There was a problem hiding this comment.
Request changes: the production refactor is sound, but the test-contract migration is incomplete and normal BE-UT configurations cannot pass as submitted.
Critical checkpoint conclusions:
- Goal and proof: Centralizing the cached single-row data column removes caller-side physical-shape checks and preserves production behavior, but the changed/new tests do not prove a passing cross-build contract; two blocking test failures are called out inline.
- Scope and clarity: The change is conceptually focused. The accessor migration covers all repository consumers; the related
VMatchPredicateremoval was verified as a no-op for valid plans because MATCH retains a nonconstant slot child. - Concurrency: The wrapper is immutable after construction and existing shared pointers are populated during expression open. No new thread, lock, atomic, dependency-order, or deadlock behavior was introduced; valid scan constant-false paths still signal readiness as before.
- Lifecycle, memory, and nullability: The wrapper owns the unwrapped
ColumnPtr;FunctionContextclones share that ownership, so cachedStringRefvalues remain live. Nullable and array/map/struct row-zero semantics and COW ownership are preserved, with no new untracked allocation or reference cycle. - Configuration and compatibility: No configuration, FE-BE variable, serialization, storage-format, or rolling-upgrade contract changes. The internal source API migration is complete across the repository.
- Parallel paths and special checks: File, mock, OLAP, generic scan, scalar-function, collection-IN, constant rebroadcast, and ANN paths were traced. The one-row check is a valid production invariant, but its direct test/mock callers and build-mode-specific failure behavior were not fully migrated.
- Test coverage and results: Existing scan-normalization cases now fail during setup, and the new unconditional death test fails under
NDEBUG. The visible macOS BE check is compile-only withMAKE_TEST=OFF, so it does not cover either issue. No local builds or tests were run because this review runner prohibits builds. - Observability: No new runtime path requires logs, metrics, or tracing.
- Transactions, persistence, and writes: Not applicable; the PR changes neither transaction state nor data-write/persistence behavior.
- Performance: Constant unwrapping remains no-copy and is performed once at wrapper construction; no hot-path regression or redundant allocation was found.
- Additional user focus: none was provided. The complete PR was reviewed without further narrowing.
Review completion: converged in Round 2 with two inline findings and no unresolved candidate.
| } else { | ||
| _column_ptr = std::move(column); | ||
| } | ||
| DORIS_CHECK_EQ(_column_ptr->size(), 1); |
There was a problem hiding this comment.
[P1] Update the existing two-row mock-constant tests
This invariant is reached immediately by the existing non-disabled ScanNormalizePredicate.test_eval_const_conjuncts3 and test_eval_const_conjuncts4: lines 130 and 154 pass two-row non-const columns through MockFnCall::set_const_expr_col(), which constructs this wrapper outside any death/exception assertion. The normal BE-UT CMake glob includes exec/*.cpp, so those tests now abort or throw before reaching their remaining code. (The green macOS check only builds BE with MAKE_TEST=OFF.) Please update/remove those malformed fixtures, or assert the new failure at construction, together with this invariant.
|
|
||
| TEST(ColumnPtrWrapperTest, RejectsNonConstMultiRowColumn) { | ||
| auto column = ColumnHelper::create_column<DataTypeInt64>({7, 8}); | ||
| EXPECT_DEATH(static_cast<void>(ColumnPtrWrapper(column)), ""); |
There was a problem hiding this comment.
[P1] Assert the release-mode exception path here
This only behaves as a death assertion while DCHECKs are enabled. With NDEBUG, DORIS_CHECK_EQ uses the release comparison macro and doris_check_fail() throws doris::Exception; GoogleTest does not count leaving a death-test statement by exception as death. The existing be/test/common/check_test.cpp handles this contract by capturing the exception under NDEBUG and using EXPECT_DEATH only under DCHECK_IS_ON(). Please mirror that split (or use a build-mode-neutral helper), otherwise release BE-UT fails this new test.
|
run buildall |
What problem does this PR solve?
Cached constant expressions could expose either a
ColumnConstor a single-row data column, forcing callers to inspect the physical column type. This change makesColumnPtrWrapperstore a private, single-row data column, unwrapsColumnConstwithout copying, and createsColumnConstonly when broadcasting the cached value. It also removes redundant constant evaluation fromVMatchPredicate::open().Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)