[refine](column) use strong-typed ColumnUInt8 for null_map in ColumnNullable#63491
Merged
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
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.
Review result: no blocking findings found.
Critical checkpoint conclusions:
- Goal and tests: The PR is a focused refactor to strongly type
ColumnNullablenull maps asColumnUInt8; the changed code appears to accomplish that goal. Existing CI format checks pass; the macOS BE UT failure is an environment JDK mismatch, not code-related. No new test is strictly required for this narrow type refactor, but a BE compile/UT run is still the main residual validation. - Scope and clarity: The change is small and mostly limited to
ColumnNullable, COW alias exposure, and direct call-site cast removal. - Concurrency: No new shared mutable state, locks, threads, or async paths are introduced.
- Lifecycle/static initialization: No static/global lifecycle changes were introduced.
- Configuration: No configuration items were added or changed.
- Compatibility: No storage format, protocol, function symbol, or FE/BE compatibility impact found.
- Parallel code paths: Reviewed representative null-map access call sites and no missed required analogous change was found.
- Conditions/error handling: Constructor validation continues to reject constant null maps and enforces the
ColumnUInt8invariant at the construction boundary. - Test coverage/results: No test-result files changed. CI status shows style checks passed; the only observed failed check is unrelated runner JDK setup.
- Observability: Not applicable for this internal column representation refactor.
- Transaction/persistence/data writes: Not applicable; no transaction, persistence, or data visibility paths changed.
- Performance: The refactor removes repeated null-map downcasts on hot accessors and does not introduce obvious extra work.
User focus: No additional user-provided review focus was specified.
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 31717 ms |
Contributor
TPC-DS: Total hot run time: 171321 ms |
Mryange
force-pushed
the
ColumnUInt8-for-nullmap
branch
from
669ebf2 to
5be824f
Compare
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Review result: request changes.
Critical checkpoint conclusions:
- Goal/test: The PR aims to make ColumnNullable's null map strongly typed as ColumnUInt8. The direction is reasonable, but the PR description does not report any executed test, and I could not verify formatter compliance because this runner lacks clang-format 16.
- Scope/focus: The core type change is focused, but the downstream call-site edits introduced widespread unformatted code.
- Concurrency/lifecycle: No new concurrency, lock ordering, or non-obvious lifecycle ownership issue was identified in the reviewed paths. The COW ownership model remains the main relevant lifecycle concern.
- Configuration/compatibility/protocol: No new configuration, storage format, or FE-BE protocol compatibility change was identified.
- Parallel code paths: The null-map accessor call sites were updated broadly across column, function, table-function, and segment reader paths.
- Error handling/data correctness: I did not identify an additional ignored Status or visibility/delete-bitmap correctness issue in this refactor.
- Tests: No test execution is documented in the PR body. This refactor should at minimum pass BE build/style and relevant column/table-function tests.
- Observability/performance: No new observability requirement. The intended performance improvement is removal of redundant casts, but it must not land with formatter violations.
User focus: No additional user-provided review focus was present.
Blocking issue: BE C++ formatting has not been applied. Several added lines are visibly mis-indented across touched files, which violates the repository code-style requirement. Please run the Doris BE formatter (build-support/clang-format.sh) with clang-format 16 and update the PR.
| @@ -340,11 +339,10 @@ | |||
| DCHECK(value_col_ptr->is_nullable()) | |||
| << "posexplode fast path requires nullable value column"; | |||
| auto* val_nullable = assert_cast<ColumnNullable*>(value_col_ptr); | |||
Contributor
There was a problem hiding this comment.
This added line is over-indented relative to the surrounding block. The same formatting drift appears in several touched BE files, so please run the required Doris formatter (build-support/clang-format.sh with clang-format 16) rather than fixing only this line.
| assert_cast<ColumnNullable*>(column.get())->get_nested_column_ptr().get()) | ||
| ->insert_range_from(*_elements_column, _cur_offset, max_step); | ||
| static_cast<ColumnUInt8*>( | ||
| static_cast<ColumnNullable*>(column.get())->get_null_map_column_ptr().get()) |
Contributor
There was a problem hiding this comment.
This block is no longer clang-formatted: the statements inside if (_is_nullable) are aligned with the if instead of being indented under it. Please run the BE formatter for the whole PR.
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
I found a blocking correctness regression in the ColumnNullable refactor. The typed null-map storage is a good direction, but the adapter used in for_each_subcolumn breaks existing read-only traversal paths. Existing formatting comments were already present and are not repeated here.
Critical checkpoint conclusions:
- Goal/test: The PR aims to make ColumnNullable null maps strongly typed as ColumnUInt8. The core goal is only partially met because one traversal path now fails; no PR test demonstrates recursive column traversal on Nullable columns.
- Scope: The actual PR diff is focused on the null-map refactor, aside from formatting drift already reported in existing review threads.
- Concurrency/lifecycle/config/compatibility: No new concurrency, lifecycle, config, or serialization-format compatibility concern found in the actual PR diff.
- Parallel paths: Most null-map access call sites were updated consistently, but for_each_subcolumn needs a safe typed-to-base adapter.
- Testing: Missing coverage for IColumn::dump_structure(), null_map_check(), column_boolean_check(), and recursive mutate/traversal on ColumnNullable after this type change.
- Data correctness/performance/observability: No data visibility or persistence issue found; the reported issue is a runtime correctness regression in column traversal.
User focus: No additional user-provided review focus was specified.
| callback(_nested_column); | ||
| callback(_null_map); | ||
| IColumn::WrappedPtr null_map(static_cast<const ColumnUInt8::Ptr&>(_null_map)); | ||
| callback(null_map); |
Contributor
There was a problem hiding this comment.
This temporary wrapper makes read-only subcolumn traversal fail for nullable columns. IColumn::dump_structure(), count_const_column(), column_boolean_check(), and null_map_check() all call for_each_subcolumn with callbacks that only inspect the subcolumn and do not replace it. In those paths this local null_map still aliases _null_map after callback(null_map), so the final null_map.get() goes through non-const chameleon_ptr::get() and calls assert_mutable_ref() with use_count() > 1, throwing COW::assert_mutable. For example, calling dump_structure() on a ColumnNullable will now fail just because the traversal copied the null-map pointer. Please avoid creating an extra owner for the no-replacement case, or otherwise update _null_map only through a path that does not require exclusive ownership when the callback did not detach/replace the pointer.
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Reviewed the current PR diff against the Doris code review checklist. I did not find additional non-duplicate blocking issues beyond the already-open review context. The earlier formatting concerns appear addressed in the latest head, and the nullable subcolumn traversal path now avoids updating the null-map wrapper when the callback leaves it unchanged.
Critical checkpoint conclusions:
- Goal and coverage: the PR narrows null-map storage/access to and updates callers accordingly. The implementation is focused on COW/type-safety cleanup; I did not see new functional tests in this diff, and I did not run the full BE test suites locally.
- Scope and clarity: the change is mostly mechanical and focused on nullable null-map typing plus necessary call-site updates.
- Concurrency/lifecycle: no new threads, shared mutable globals, lock ordering, or non-obvious lifecycle management were introduced in the reviewed diff.
- Configuration/compatibility: no new configs, serialization formats, persistent metadata, or FE/BE protocol fields are added.
- Parallel paths: table-function, expression, and storage reader call sites that directly touched nullable null maps were updated consistently in the PR diff.
- Error handling/invariants: the new null-map type checks fail fast for non- null maps, matching Doris invariants.
- Data correctness: storage reader changes preserve status propagation and still append into the destination null-map column; I did not identify a version/delete-bitmap/transaction visibility impact.
- Performance/memory: no obvious new large untracked allocation or hot-path regression was identified; COW mutation paths still detach subcolumns through .
- Observability: no new operational path requiring logs or metrics was introduced.
User focus: no additional user-provided review focus was specified.
Contributor
There was a problem hiding this comment.
Reviewed the current PR diff against the Doris code review checklist. I did not find additional non-duplicate blocking issues beyond the already-open review context. The earlier formatting concerns appear addressed in the latest head, and the nullable subcolumn traversal path now avoids updating the null-map wrapper when the callback leaves it unchanged.
Critical checkpoint conclusions:
- Goal and coverage: the PR narrows ColumnNullable null-map storage/access to ColumnUInt8 and updates callers accordingly. The implementation is focused on COW/type-safety cleanup; I did not see new functional tests in this diff, and I did not run the full BE test suites locally.
- Scope and clarity: the change is mostly mechanical and focused on nullable null-map typing plus necessary call-site updates.
- Concurrency/lifecycle: no new threads, shared mutable globals, lock ordering, or non-obvious lifecycle management were introduced in the reviewed diff.
- Configuration/compatibility: no new configs, serialization formats, persistent metadata, or FE/BE protocol fields are added.
- Parallel paths: table-function, expression, and storage reader call sites that directly touched nullable null maps were updated consistently in the PR diff.
- Error handling/invariants: the new null-map type checks fail fast for non-ColumnUInt8 null maps, matching Doris invariants.
- Data correctness: storage reader changes preserve status propagation and still append into the destination null-map column; I did not identify a version/delete-bitmap/transaction visibility impact.
- Performance/memory: no obvious new large untracked allocation or hot-path regression was identified; COW mutation paths still detach subcolumns through for_each_subcolumn.
- Observability: no new operational path requiring logs or metrics was introduced.
User focus: no additional user-provided review focus was specified.
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 32295 ms |
Contributor
TPC-DS: Total hot run time: 177509 ms |
Mryange
force-pushed
the
ColumnUInt8-for-nullmap
branch
from
32e7ee1 to
3ef87ba
Compare
Mryange
force-pushed
the
ColumnUInt8-for-nullmap
branch
from
3ef87ba to
1eeeefb
Compare
Contributor
Author
|
/review |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
I found one additional blocking COW issue beyond the existing review threads.
Critical checkpoint conclusions:
- Goal/test coverage: the PR appears to tighten ColumnNullable null-map typing and update call sites/tests, but the mutable constructor path is not covered for nested nullable inputs with a generic MutableColumnPtr null map.
- Scope/focus: mostly focused, but the constructor ownership change is not a safe minimal refactor because it changes rvalue consumption behavior.
- Concurrency/lifecycle/config/compatibility: no new thread concurrency, config, storage-format, or FE/BE protocol compatibility issue found in the reviewed diff.
- Parallel paths: storage reader/table-function/expression call sites were checked; no distinct issue found there beyond the already raised formatting threads.
- Error handling/memory/COW: the new mutable null-map cast can leave multiple owners and then call non-const WrappedPtr access that requires exclusive ownership, causing valid construction to fail.
- Tests: existing tests do not appear to exercise this constructor failure mode. Regression output changes were reviewed; no additional issue found.
- Observability/performance: no additional observability or performance blocker found.
User focus: no additional user-provided review focus was specified.
| nested_nullable.apply_null_map(get_null_map_column()); | ||
| _null_map = nested_nullable._null_map; | ||
| _nested_column = nested_nullable.get_nested_column_ptr(); | ||
| } |
Contributor
There was a problem hiding this comment.
This delegating constructor no longer consumes the rvalue null-map pointer. assert_mutable_null_map(null_map_) creates a ColumnUInt8::MutablePtr from the raw pointer while the original MutableColumnPtr argument still owns the same object. When the nested column is already nullable, the delegated constructor immediately calls get_null_map_column() in the flattening path, and the non-const ColumnUInt8::WrappedPtr access asserts exclusive ownership; with the original argument plus _null_map, use_count() is greater than 1 and construction can throw COW::assert_mutable. Even when that path is not taken, callers passing std::move(null_map) are left with a still-owning pointer, which is a behavioral change from the old std::move(null_map_) member initialization. Please make the helper consume/reset the source MutableColumnPtr after type-checking (or otherwise move through a typed owner) so the constructed ColumnNullable is the sole owner before any non-const _null_map access.
Contributor
There was a problem hiding this comment.
Requesting changes for one COW ownership regression in the nullable-column refactor.
Critical checkpoint conclusions:
- Goal/test: The goal is to make ColumnNullable null maps strongly typed as ColumnUInt8. The code mostly follows that goal, but the mutable constructor path breaks nested nullable flattening; I did not find a test covering this path.
- Scope: The PR is focused, but the constructor ownership conversion is not small/safe enough because it creates an extra owner before a mutable access.
- Concurrency: No new thread/concurrency behavior found.
- Lifecycle/COW: This change is lifecycle-sensitive due intrusive COW ownership; the reported issue is a refcount/lifetime regression.
- Config/compatibility/persistence: No new configs, protocol/storage-format compatibility, or persistence changes found.
- Parallel paths: The shared/const constructor path handles nested nullable separately; the distinct mutable constructor path has the reported failure.
- Tests: Existing tests were only adjusted for API usage; coverage for nested nullable construction with a MutableColumnPtr null map is missing.
- Observability/performance: No additional observability requirement found; the performance intent is reasonable once the ownership bug is fixed.
Existing review context was considered. I did not duplicate the existing for_each_subcolumn or formatting threads. User focus: no additional user-provided focus points.
| throw doris::Exception(ErrorCode::INTERNAL_ERROR, | ||
| "ColumnNullable cannot have constant null map"); | ||
| __builtin_unreachable(); | ||
| auto& nested_nullable = assert_cast<ColumnNullable&>(*_nested_column); |
Contributor
There was a problem hiding this comment.
This can throw when the mutable constructor is used to flatten Nullable(Nullable(T)). The delegating constructor above calls assert_mutable_null_map(null_map_) while null_map_ still owns the same ColumnUInt8, so _null_map has an extra owner until the delegating constructor parameter is destroyed. If _nested_column is nullable, this non-const get_null_map_column() goes through ColumnUInt8::WrappedPtr::operator*() and assert_mutable_ref(), sees use_count() > 1, and fails before the null maps can be merged. A valid path is ColumnNullable::create(std::move(nullable_nested), std::move(null_map_as_mutable_column_ptr)). Please consume/move the original MutableColumnPtr before creating the typed owner, or pass the null map to apply_null_map through a const path that does not require exclusive ownership.
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Review result: no new blocking issues found in the current head beyond the already-known inline threads.
Critical checkpoint conclusions:
- Goal/test proof: The PR changes
ColumnNullablenull-map storage to a concreteColumnUInt8::WrappedPtrand updates callers. The code is consistent with that goal. Existing BE/unit coverage was adjusted at call sites; I did not run full BE tests in this runner. - Scope/focus: The change is mostly focused on the null-map type refactor and cast removal.
- Concurrency/lifecycle: No new thread, static initialization, or lock lifecycle changes found. COW ownership paths were the main risk area reviewed.
- Configuration/compatibility: No config, storage format, or FE-BE protocol compatibility changes found.
- Parallel paths: Reviewed core column accessors plus function, table-function, and segment-reader call sites that consume nullable null maps; no distinct missing path found.
- Error handling: Existing status-returning iterator calls remain checked. Constructor validation keeps null-map type/size checks.
- Data correctness: Null-map propagation in nullable constructor flattening, conditional functions, table functions, and array/map/struct readers appears version-neutral and data-shape preserving.
- Memory/performance: The refactor removes repeated null-map casts and does not add obvious hot-path allocations beyond existing pointer conversions.
- Observability: No new observability needed for this internal refactor.
- Tests: Existing touched tests are formatting/API updates rather than new behavioral coverage; full verification is still dependent on CI.
User focus: no additional user-provided review focus was specified.
Contributor
Author
|
run buildall |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-H: Total hot run time: 31738 ms |
Contributor
TPC-DS: Total hot run time: 171136 ms |
zclllyybb
approved these changes
May 26, 2026
yiguolei
approved these changes
May 26, 2026
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
5 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?
The null_map column in
ColumnNullablewas stored as a genericIColumn::WrappedPtr, requiringassert_cast<ColumnUInt8>at every call site that accessed it. This had two problems:get_null_map_column()orget_null_map_column_ptr()performed anassert_cast, adding unnecessary overhead on hot paths (filter evaluation, null propagation, etc.).WrappedPtrtype provided no compile-time guarantee that the null map is always aColumnUInt8, making it possible to accidentally store a wrong column type.Root cause: The
_null_mapmember was typed as a genericIColumn::WrappedPtrinstead of the concreteColumnUInt8::WrappedPtr.Fix: Change
_null_maptoColumnUInt8::WrappedPtrso the type is fixed at the member level. The constructor now validates and converts toColumnUInt8::MutablePtron entry via a helperassert_mutable_null_map(). All call sites that previously didassert_cast<ColumnUInt8>now get the correct type directly.Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)