[opt](column) Reuse varbinary payload storage for derived columns#65217
[opt](column) Reuse varbinary payload storage for derived columns#65217Mryange wants to merge 3 commits into
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
| using Container = PaddedPODArray<doris::StringView>; | ||
|
|
||
| private: | ||
| struct Storage { |
There was a problem hiding this comment.
这个东西直接用Arena就行了吧，还多套一层？感觉有点抽象啊
github-actions
Bot
added
the
approved
|
PR approved by at least one committer and no changes requested. |
|
PR approved by anyone and no changes requested. |
Mryange
force-pushed
the
opt-column-varbinary-storage
branch
from
3bdef28 to
4efb4d2
Compare
|
/review |
github-actions
Bot
removed
the
approved
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
I found one issue that should be fixed before merge: the new retained-owner path keeps source VARBINARY payload memory alive, but the column/block memory accounting no longer reports that retained memory.
Critical checkpoint conclusions:
- Goal and tests: the PR targets avoiding deep copies while preserving filtered/permuted VARBINARY payload lifetime. The new tests cover readability after source clear, but not retained-memory accounting.
- Scope: the code change is small and focused on ColumnVarbinary ownership plus unit coverage.
- Concurrency: no new threads or shared mutable concurrent state were introduced.
- Lifecycle: the main lifecycle change is source Arena retention through AuxiliaryDataSet; this is where the accepted accounting issue appears.
- Config and compatibility: no new config, persistence, storage format, or FE/BE protocol compatibility concern found.
- Parallel paths: filter and permute use the new shallow-copy retention path; insert/range/selector/direct varbinary writers still allocate or copy through the local column arena.
- Tests and style: the new BE UT files are picked up by the recursive test glob. I did not run BE UTs because this review checkout lacks .worktree_initialized, thirdparty/installed, and thirdparty/installed/bin/protoc.
- Observability, transactions, data writes: not applicable to this BE core column change.
- Performance and memory: request changes for the under-reported retained memory described inline.
User focus: no additional user-provided review focus was supplied.
Subagent conclusions: optimizer-rewrite reported OPT-1 and tests-session-config reported TSC-001 for the same retained-memory accounting bug; I merged them as MAIN-001 and submitted it as the single inline comment. No candidate was dismissed except as a duplicate of MAIN-001. Convergence round 1 ended with both live subagents replying NO_NEW_VALUABLE_FINDINGS for this exact ledger/comment set.
|
|
||
| size_t allocated_bytes() const override { return _data.allocated_bytes() + _arena.size(); } | ||
| size_t allocated_bytes() const override { | ||
| DCHECK(_arena != nullptr); |
There was a problem hiding this comment.
filter() and permute() can now make the result retain the source arena through _auxiliary_data, but this accounting still only includes the result column's fresh local _arena. After a filtered/permuted block keeps a large non-inline VARBINARY payload alive and the source is cleared or reused, Block::allocated_bytes() and Block::bytes() will sum these methods and under-report the retained payload memory. That bypasses the memory counters/backpressure paths that rely on block size. Please either keep copying non-inline payloads for derived columns, or make auxiliary owners contribute to byte_size()/allocated_bytes() without double-counting.
|
/review |
There was a problem hiding this comment.
Requesting changes for one additional lifecycle/cleanup issue in ColumnVarbinary::clear().
I did not repeat the existing VARBINARY retained-memory accounting thread at discussion_r3534250446; that concern is already covered. The failed BE UT (macOS) check exits before compile at the environment gate because the runner reports JDK 25 while the job requires JDK 17, so I did not treat that as a patch-specific code issue.
Review coverage: inspected all five changed files, surrounding ColumnVarbinary, block filtering/clearing, COW, sort/permutation, join row-transfer, aggregation/hash consumer paths, BE unit-test registration, refreshed inline comments, and CI status. Static validation only: this checkout lacks .worktree_initialized, thirdparty/installed, and thirdparty/installed/bin/protoc; git diff --check is clean.
Subagent conclusions: optimizer-rewrite and tests-session-config both completed the required initial pass with NO_NEW_VALUABLE_FINDINGS, then both returned NO_NEW_VALUABLE_FINDINGS again in convergence round 1 for the final ledger/comment set containing MAIN-001. No subagent candidates were appended; the existing memory-accounting concern was marked duplicate-covered.
Critical checkpoint conclusions: the PR goal is clear and mostly focused, with unit coverage for owner retention after filter/permute. The remaining issue is a cleanup/lifecycle regression where clear() now allocates and is not exception-safe. No new concurrency, FE/BE protocol, persistence, config, or compatibility surface was introduced. Parallel block/filter/sort/join/aggregate paths were inspected for VARBINARY ownership behavior. Test files are picked up by the BE UT recursive glob, but local BE UTs could not be run in this runner due missing third-party dependencies.
| void clear() override { | ||
| _data.clear(); | ||
| _arena.clear(); | ||
| reset_to_new_local_storage(); |
There was a problem hiding this comment.
clear() is now on an allocation path because it calls reset_to_new_local_storage(), which clears the current owner set and then does std::make_shared<Arena>(). This method is reached from cleanup/reuse code such as MutableBlock::clear_column_data() noexcept (block.cpp:1286-1290), so an allocation failure while clearing a VARBINARY column can now terminate the process during cleanup; in non-noexcept callers it is also not exception-safe because _arena still points at the old Arena after _auxiliary_data.clear() destroys it if the new allocation throws. Please make the reset allocate the replacement owner before dropping the current one, and avoid introducing a throwing allocation into the clear/reuse path (for example by reusing the local Arena or by separating the local owner from borrowed owners).
|
/review |
There was a problem hiding this comment.
Automated review summary for head d32f82d.
I completed the review pass over all changed files:
be/src/core/auxiliary_data_set.hbe/src/core/block/block.cppbe/src/core/block/block.hbe/src/core/column/column_varbinary.cppbe/src/core/column/column_varbinary.hbe/test/core/auxiliary_data_set_test.cppbe/test/core/column/column_varbinary_test.cpp
I did not add new inline comments in this pass. The substantiated issues I found are already covered by existing inline threads:
- comment 3534250446 covers retained borrowed VARBINARY payload owners not being charged by
allocated_bytes()/byte_size(). - comment 3534421683 covers
ColumnVarbinary::clear()adding a new allocation path on cleanup/reuse.
Critical checkpoint conclusions:
- Goal and tests: the PR aims to keep non-inline
ColumnVarbinarypayloads alive afterfilter()/permute(). The new unit tests cover owner retention and filtered/permuted values surviving source clear. Test registration is covered by the existing recursivebe/test/CMakeLists.txtcore/*.cppglob. - Scope: the patch is focused on VARBINARY owner retention and related tests, but it changes column memory/lifecycle behavior. The two existing inline comments remain the main correctness/backpressure concerns.
- Concurrency and lifecycle: I found no new threaded state. Shared payload lifetime is represented by retained owners in
AuxiliaryDataSet; the cleanup/reset allocation risk is already tracked by comment 3534421683. - Compatibility and persistence: no config, protocol, storage format, or EditLog compatibility changes were introduced.
- Parallel paths:
insert_from,insert_range_from,insert_indices_from, andreplace_column_datastill deep-copy non-inline payloads into the destination column;filter()andpermute()are the intentional borrowed-owner paths. - Testing and CI: I did not run local BE tests because this checkout is missing
.worktree_initialized,thirdparty/installed, andthirdparty/installed/bin/protoc. I checked the currentBE UT (macOS)failure; it fails during environment setup withERROR: The JAVA version is 25, it must be JDK-17., not in the added tests. - User focus: no additional user-provided review focus was supplied.
Subagent conclusions:
optimizer-rewrite: no new valuable findings; convergence round 1 returnedNO_NEW_VALUABLE_FINDINGS.tests-session-config: recorded duplicate-only notes for comments 3534250446 and 3534421683; convergence round 1 returnedNO_NEW_VALUABLE_FINDINGS.
|
run buildall |
TPC-H: Total hot run time: 30139 ms |
TPC-DS: Total hot run time: 180194 ms |
ClickBench: Total hot run time: 24.89 s |
|
run buildall |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
3 participants
What problem does this PR solve?
ColumnVarbinary used to deep-copy non-inline payload bytes when building filtered or permuted result columns. Root cause: StringView only stores a lightweight pointer for non-inline values, while the previous column-local Arena ownership model could not keep source payload memory alive after a derived column reused those StringView entries.
This PR adds a generic AuxiliaryDataSet owner-retention helper and uses it in ColumnVarbinary. The column keeps an Arena* only as a typed, non-owning write handle; the actual Arena owner is retained by AuxiliaryDataSet. Derived filter and permute columns now copy StringView metadata and inherit the source owner set when non-inline values are present. clear() drops old owners and registers a fresh local Arena, so old payload memory can still be retained by existing derived columns without affecting new writes.
The change also adds unit coverage for AuxiliaryDataSet owner retention/de-duplication and for ColumnVarbinary derived columns remaining valid after the source column is cleared.
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)