[improvement](compaction) Support lazy chunked row ID conversion - #67333
[improvement](compaction) Support lazy chunked row ID conversion#67333mymeiyi wants to merge 1 commit into
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
|
Codex automated review failed and did not complete. Error: Selected model is at capacity. Please try a different model. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
There was a problem hiding this comment.
Pull request overview
This PR extends Doris BE compaction infrastructure to support segment-range (chunked) processing and introduces an opt-in lazy, chunked RowIdConversion mode to reduce memory usage when only sparse source row ranges are touched (a prerequisite for future parallel / sub-range compactions). It also adds cloud single-rowset grouped compaction metadata (segment groups) and corresponding readers/compaction logic, plus broad unit/regression coverage.
Changes:
- Add
RowIdConversion::Mode::LAZY_CHUNKEDto allocate row-id mappings on-demand in 4096-row chunks, and plumbStatus-returning APIs through merge paths. - Introduce
NONOVERLAPPING_WITHIN_GROUP+segment_group_sizesin rowset proto/meta and implement cloud single-rowset grouped compaction using segment-range merges. - Extend vertical/horizontal merge and
VerticalBlockReaderto respect segment ranges and grouped-segment iterator initialization, with new BE UTs and a cloud regression suite.
Checklist Conclusions (from review checkpoints)
- Goal & tests: The goal (reduce RowIdConversion memory + enable segment-range merge paths for grouped compaction) is implemented and backed by extensive UTs and a new cloud regression suite.
- Scope/focus: The PR is broad (proto + compaction execution path + readers + tests). Changes are cohesive but touch high-risk storage/compaction code paths.
- Concurrency: No new explicit concurrency primitives were introduced in the changed code; the work mainly restructures compaction execution and reader initialization. (Still high-risk due to compaction’s operational sensitivity.)
- Compatibility: Proto is extended (new enum + repeated field). Conversions/copy paths are updated for cloud metadata; rolling-upgrade behavior depends on invariants around when the new enum value can appear.
- Config: New cloud configs are added with defaults; they are used as runtime flags to gate behavior.
- Observability: A new INFO log is added for grouped compaction completion with useful tags.
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
regression-test/suites/cloud_p0/compaction/test_cloud_single_rowset_grouped_compaction.groovy |
New cloud regression coverage for grouped rowset compaction behavior and follow-up compaction to fully non-overlapping. |
gensrc/proto/olap_file.proto |
Add NONOVERLAPPING_WITHIN_GROUP and segment_group_sizes fields to rowset metadata protos. |
be/test/storage/rowset/rowset_meta_test.cpp |
UT ensuring grouped overlap is treated as “overlapping” semantics and compaction-produced behavior. |
be/test/storage/rowid_conversion_test.cpp |
UTs for lazy chunked RowIdConversion plus an end-to-end grouped-compaction rowid correctness test. |
be/test/storage/pb_convert_test.cpp |
UT validating Doris↔Cloud rowset meta conversion for segment_group_sizes. |
be/test/storage/iterator/vertical_block_reader_test.cpp |
New UT for grouped iterator initialization flags and invalid-input behavior. |
be/test/storage/cloud_file_cache_write_index_only_test.cpp |
Extend UT assertions around allocated segment ID progression. |
be/test/cloud/cloud_snapshot_mgr_test.cpp |
Update snapshot conversion UT to include grouped overlap + group sizes. |
be/test/cloud/cloud_cumulative_compaction_policy_test.cpp |
Add policy test ensuring cumulative point advancement logic with overlapping output. |
be/test/cloud/cloud_compaction_test.cpp |
Add extensive UT coverage for grouped-compaction candidate checks, merge-range construction, and selection behaviors. |
be/src/storage/rowset/vertical_beta_rowset_writer.cpp |
Adjust segment row-count tracking and reset segment-writer state after final flush. |
be/src/storage/rowset/rowset_meta.h |
Add segment-group size validation APIs and treat within-group overlap as compaction-produced/overlapping semantics. |
be/src/storage/rowid_conversion.h |
Implement LAZY_CHUNKED mode, add memory tracking hooks, and return Status from add(). |
be/src/storage/merger.h / be/src/storage/merger.cpp |
Add optional segment_range plumbing and propagate Status from rowid conversion updates. |
be/src/storage/iterator/vertical_block_reader.h / be/src/storage/iterator/vertical_block_reader.cpp |
Add grouped overlap iterator-init logic and make iterator-init flags respect grouped layouts and segment ranges. |
be/src/storage/compaction/compaction.h / be/src/storage/compaction/compaction.cpp |
Refactor merge into overridable stages and add segment-range merge execution helper with progress mapping. |
be/src/cloud/pb_convert.cpp |
Copy/swap segment_group_sizes in Doris↔Cloud rowset meta conversion. |
be/src/cloud/config.h / be/src/cloud/config.cpp |
Add cloud single-rowset compaction feature/config knobs with defaults. |
be/src/cloud/cloud_snapshot_mgr.cpp |
Copy segment_group_sizes when creating snapshot rowset metas. |
be/src/cloud/cloud_schema_change_job.cpp |
Force conservative overlap semantics when schema change rewrites grouped rowsets. |
be/src/cloud/cloud_cumulative_compaction.h / be/src/cloud/cloud_cumulative_compaction.cpp |
Implement grouped single-rowset compaction flow with segment-range merges and output group-size metadata. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
run buildall |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
### What problem does this PR solve? Issue Number: close #xxx Related PR: #xxx Problem Summary: Row ID conversion previously allocated a dense destination entry for every source row, which wastes memory when a compaction reads only sparse source ranges. Add an opt-in lazy chunked mode that allocates mappings only for touched rows, preserves dense mode for existing callers, and propagates allocation failures through merge paths. ### Release note None ### Check List (For Author) - Test: Not run (per request) - Behavior changed: No - Does this need documentation: No
ab94fb5 to
b82d2b8
Compare
|
/review |
There was a problem hiding this comment.
Review status: complete after 2 convergence rounds. No blocking or distinct actionable issue remains on head b82d2b8.
Critical checkpoint conclusions:
- Goal and proof: The PR adds an opt-in lazy 4096-row chunk representation for sparse RowIdConversion mappings and propagates allocation failure through both merge paths. The focused unit tests cover sparse touched rows, chunk boundaries, repeated segment registration, multiple rowsets and physical segment IDs, destination-segment transitions, and absent mappings.
- Scope: The change is focused to RowIdConversion, the two merger call sites, and its unit tests.
- Data correctness and MoW: Dense and lazy add/get semantics match for sentinels, untouched rows, grouped reader reopening, non-contiguous physical source IDs, and destination segment positions. Local and cloud delete-bitmap consumers resolve stored destination positions through the built output rowset before using physical IDs.
- Concurrency and lifecycle: Current production compaction still constructs DENSE and current grouped execution is sequential; no lazy object is concurrently mutated. Compaction owns and destroys the conversion under its memory tracker. A lazy allocation failure aborts the one-shot compaction, with no retry or downstream consumer using partial state.
- Memory and performance: Lazy pointer tables plus on-demand 32 KiB chunks avoid the dense per-source-row allocation for sparse ranges. The current jemalloc hook only routes allocation and does not account standard containers, so the explicit lazy byte charge is the single intended tracker charge and is released under the owning tracker.
- Compatibility and parallel paths: Default dense behavior and the dense-only inverted/SNII matrix consumer are unchanged. Both horizontal and vertical merge paths propagate Status. No protocol, persisted storage format, configuration, or FE-BE variable is changed.
- Conditions and observability: Bounds and missing mappings fail explicitly; the memory-limit Status includes process and tracker context. No new runtime metric is necessary for this opt-in data-structure mode.
- Tests and checks: No BE build or unit test was executed by this review because the review runner instructions prohibit builds. GitHub Clang Formatter and License Check passed; read-only git diff --check and build-support/check-build-hygiene.sh also passed. The BE build/test workflow jobs were skipped, so this review does not claim runtime test execution.
- User focus: No additional user-provided focus was supplied.
The sole provisional memory-accounting concern from Round 1 was dismissed in Round 2 after current executable hook code showed that older comments describing automatic tracker attribution are stale. No inline comment is warranted.
|
run buildall |
|
run nonConcurrent |
Problem Summary:
Later, we will support parallel compaction, which means one compaction sub task only handles somes rows of the input rowsets.
Now, Row ID conversion allocated a dense destination entry for every source row, which wastes memory when a compaction reads only sparse source ranges.
Add an opt-in lazy chunked mode that allocates mappings only for touched rows.