[opt](be) Avoid redundant column copies - #66436
Open
Mryange wants to merge 2 commits into
Open
Conversation
### What problem does this PR solve? Issue Number: None Related PR: apache#65367 Problem Summary: Several execution paths copied complete column data even though the source columns could remain immutable and rely on copy-on-write before later mutation. Reuse expression results and array offsets directly, publish scanner columns after validation, and let partition filtering create only the selected rows. ### Release note None ### Check List (For Author) - Test: Not run (build and test validation deferred) - Behavior changed: No - Does this need documentation: No
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 opinion: no blocking issue found. The copy-elision changes are consistent with Doris's current recursive column COW contract, and the review converged with zero inline findings.
Critical checkpoint conclusions:
- Goal and proof: the PR removes eager full-column, projection, partition-filter, array-offset, and IF-branch copies while preserving values, row counts, nullability, and destination type metadata. Existing core COW and affected feature suites cover the surrounding mechanisms, but this PR adds no focused ownership regression test.
- Scope and clarity: the ten-file change is cohesive and limited to publishing immutable column owners, validating their shape, and deferring copies until an actual mutation.
- Data correctness and errors: duplicate/reordered outputs remain visibly shared; block clear, filter, shrink, and scoped mutation detach before writing. New size/physical-shape invariants fail loudly, and no
Statusis newly ignored. - Concurrency and locks: multicast installs each consumer's block owner under its existing mutex before queue release. The patch adds no shared mutable state, lock scope, lock-order change, or deadlock surface.
- Lifecycle and ownership: FileScanner's scoped destination restores every pre-publication error path and publishes all owners before clearing its separate source block. Schema-scanner async work is dependency-fenced. Array-map retains the source offsets owner, and recursive mutation detaches nested data, offsets, and null maps. Hive/Iceberg transformers consume blocks synchronously; sorted Iceberg owns only its filtered copy.
- Configuration and observability: no configuration, dynamic-update behavior, metrics, or logging contract changes; no additional observability is needed for this ownership-only optimization.
- Compatibility and parallel paths: the changed API is internal C++, all call sites were checked, and no FE/BE protocol, storage format, function symbol, or rolling-upgrade contract changes. Hive, Iceberg, MaxCompute, multicast, schema scan, load FileScanner, array-map, and IF paths were each traced through their downstream consumers.
- Data writes, transactions, and persistence: partition masks still produce independent selected blocks for each writer, including zero/all/selective masks and writer rollover. Commit/close, transaction, and persistence behavior is unchanged.
- Performance: the change removes eager full-data copies and performs return-new filtering only when sharing requires it; no new row-wise loop, pathological allocation loop, or heavy work under a lock was introduced.
- Tests/results: no build or test was run by this review because the review prompt is static-only. GitHub's formatter and repository metadata checks are green; macOS BE UT was still in progress at final sweep. No test result is claimed here.
- Additional user focus: none was provided, so the complete PR was reviewed without a narrower focus.
Review completion: one convergence round completed with three normal full-review sections and two separate risk-focused sections, all returning NO_NEW_VALUABLE_FINDINGS. The authoritative and final live thread sets contain no existing inline issue, and the final inline comment set is empty.
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 28750 ms |
Contributor
TPC-DS: Total hot run time: 165729 ms |
Contributor
ClickBench: Total hot run time: 23.76 s |
### What problem does this PR solve? Issue Number: None Related PR: apache#65367 Problem Summary: Directly publishing file scanner expression results bypassed the insertion step that normalizes scalar Variant columns into destination Variant columns. Variant JSON stream loads therefore aborted on a physical column shape mismatch. Keep direct publication for matching column shapes and fall back to typed insertion when normalization is required. ### Release note None ### Check List (For Author) - Test: Not run (failure diagnosed from the inverted-index Variant stream-load regression log) - Behavior changed: No - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29108 ms |
Contributor
TPC-DS: Total hot run time: 165665 ms |
Contributor
ClickBench: Total hot run time: 24.12 s |
HappenLee
reviewed
Aug 7, 2026
| column_ptr = make_nullable(column_ptr); | ||
| } | ||
| mutable_output_columns[j]->insert_range_from(*column_ptr, 0, rows); | ||
| DORIS_CHECK_EQ(column_ptr->size(), rows); |
Contributor
There was a problem hiding this comment.
这个代码，你找文强确认一下，你确定这个直接用名字等于这个方式靠谱吗。很多那种列的这个名字，可能都是一些默认值，或者一些其他东西，这种我感觉容易翻车
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?
Problem Summary: Several execution paths copied complete column data even though the source columns could remain immutable and rely on copy-on-write before later mutation. Reuse expression results and array offsets directly, publish scanner columns after validation, and let partition filtering create only the selected rows.
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)