[fix](be) Materialize const columns before block merge#65770
Open
Gabriel39 wants to merge 1 commit into
Open
Conversation
### What problem does this PR solve?
Issue Number: None
Problem Summary:
MutableBlock can retain ColumnConst destinations while merge materializes incoming columns. Appending a full column to a const destination fails. Normalize const destinations once before appending.
### Release note
Fix block merge failures when scanner padding produces constant nullable columns.
### Check List (For Author)
- Test
- Unit Test
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.
Request changes: the merge-specific fix is sound, but the same const-representation mismatch remains in the functionally parallel MutableBlock row-append APIs; see the inline P1.
Critical checkpoint conclusions
- Goal and data correctness: The new helper fixes the reported
merge()andmerge_ignore_overflow()failure for a live const-nullable destination, and the added unit test proves those two paths. The broader accumulation contract is not yet correct becauseadd_row()and bothadd_rows()overloads can retain/reject per-block constants. - Scope and clarity: The patch is small and clear, but it is incomplete relative to the parallel
MutableBlockappend surface. A shared const-aware append abstraction would keep the fix focused and prevent the paths from diverging again. - Concurrency: No new shared state, thread entry, lock, atomic, or lock-order behavior is introduced. The examined mutable blocks are locally owned by their accumulation paths.
- Lifecycle and COW: The changed merge helper is safe: recursive COW acquisition detaches shared owners, conversion constructs a new full column before assignment, and
ScopedMutableBlockrestores the replacement on normal, error, and exception exits. No static-initialization or ownership-cycle issue was found. - Configuration: No configuration item or dynamic-reload behavior is involved.
- Compatibility: No symbol, protocol, storage-format, or rolling-upgrade compatibility change is involved.
- Parallel paths: This is the blocking gap.
clone_empty()preservesColumnConstrepresentation/value, while persistent partitioned-join and local-exchange accumulators calladd_rows()directly. Later blocks can therefore fail or retain a stale per-block constant. - Conditional checks: The top-level
is_column_const()check is necessary and consistent with the representation invariant; no speculative defensive branch was added. - Test coverage: The new BE unit test faithfully covers both merge wrappers, RAII restoration, row count, and null values. It needs companion coverage for repeated
add_rows()calls with different constants and const sources. - Test results: No local build or test was run because this review environment explicitly prohibits builds. GitHub clang-format and CheckStyle checks passed. The macOS BE-UT job stopped before compilation/tests because the runner had Java 25 instead of required JDK 17; Linux BE-UT and compile checks were still running at review time.
- Observability: No new logging or metric is needed for this local representation fix.
- Transactions and persistence: No transaction, EditLog, replay, visibility-version, or persisted-state path is involved.
- Data writes and crash behavior: No storage write/commit path is changed; the relevant correctness issue is in in-memory block accumulation.
- FE-BE propagation: No new FE/BE variable, thrift field, or scattered send path is involved.
- Performance: Destination materialization occurs only while the destination remains const and is required before full-column append. No additional hot-path regression was found, but the parallel-path fix should avoid repeatedly materializing the same owner.
- Other issues: No additional distinct correctness, memory-safety, error-handling, nullable-shape, or test-result issue survived verification and convergence.
User focus
No additional user-provided focus was specified; the full PR and its upstream/downstream append paths were reviewed.
| DataTypes _data_types; | ||
| std::vector<std::string> _names; | ||
|
|
||
| void materialize_const_column(size_t position) { |
Contributor
There was a problem hiding this comment.
[P1] Make the row-append APIs const-aware too
This helper is only called by the two merge paths, but MutableBlock::add_row() and both add_rows() overloads still append directly to _columns. Those destinations can also remain ColumnConst: for example, _partition_block() seeds a persistent buffer with in_block->clone_empty(), and ColumnConst::clone_resized(0) preserves the first block's nested value. After the first append, a later valid block with the same type but a different constant value is rejected by ColumnConst::insert_indices_from() (and a full source is also rejected by the sibling append methods). Please extend the shared append handling so both the destination and any const source are materialized, or are otherwise handled without assuming one per-block value persists across later blocks, and cover two add_rows() calls with different constants. Otherwise the same representation mismatch still breaks partitioned-join/local-exchange accumulation.
Contributor
TPC-H: Total hot run time: 29489 ms |
Contributor
TPC-DS: Total hot run time: 177854 ms |
Contributor
ClickBench: Total hot run time: 25.08 s |
Gabriel39
marked this pull request as ready for review
July 17, 2026 15:07
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
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
Related PR: None
Related Jira: DORIS-27180
Problem Summary:
ScopedMutableBlockcan retain aColumnConstdestination, while block merge materializes the incoming column before appending it. Appending that full column to the const destination fails with an internal error.This change materializes a const destination once before appending, keeping the destination and source representations compatible for both regular and ignore-overflow merges.
Release note
Fix block merge failures when scanner padding produces constant nullable columns.
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)