Skip to content

[improvement](be) Persist rowset key bounds for non-MoW rowsets - #62457

Closed
liaoxin01 wants to merge 2 commits into
apache:masterfrom
liaoxin01:liaoxin/non-mow-rowset-key-bounds-compat
Closed

[improvement](be) Persist rowset key bounds for non-MoW rowsets#62457
liaoxin01 wants to merge 2 commits into
apache:masterfrom
liaoxin01:liaoxin/non-mow-rowset-key-bounds-compat

Conversation

@liaoxin01

@liaoxin01liaoxin01 commented Apr 13, 2026

Copy link
Copy Markdown
Contributor

What problem does this PR solve?

Issue Number: None

Related PR: None

Problem Summary: Persist rowset-level key bounds for non-MoW rowsets to reduce rowset meta memory usage and shrink the meta value stored in FoundationDB, avoiding value-size limit issues caused by full per-segment key bounds. The change keeps full segment key bounds for MoW rowsets and preserves the related compaction, snapshot, cloud conversion, and recycle flows.

Release note

None

Check List (For Author)

  • Test: Attempted BE unit test and targeted object compilation
    • Unit Test / Manual test
  • Behavior changed: Yes (non-MoW rowsets can persist rowset-level key bounds instead of full per-segment key bounds; this branch temporarily defaults enable_rowset_key_bounds_for_non_mow to true for CI, while the path remains config-gated)
  • Does this need documentation: No

CopilotAI review requested due to automatic review settings April 13, 2026 15:49
@Thearas

Copy link
Copy Markdown
Contributor

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR.

Please clearly describe your PR:

  1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
  2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
  3. What features were added. Why was this function added?
  4. Which code was refactored and why was this part of the code refactored?
  5. Which functions were optimized and what is the difference before and after the optimization?

@liaoxin01
liaoxin01force-pushed the liaoxin/non-mow-rowset-key-bounds-compat branch from aaac837 to b91d64cCompareApril 13, 2026 15:50
@liaoxin01liaoxin01 changed the title [improvement](be) Persist non-MoW rowset key bounds behind config[improvement](be) Persist rowset key bounds for non-MoW rowsetsApr 13, 2026

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds support for persisting rowset-level key bounds for non-MoW rowsets and threads the new field through BE + cloud meta paths, with the “persist only rowset-level bounds (omit per-segment bounds)” behavior gated by a disabled-by-default BE config.

Changes:

  • Extend rowset meta protos with rowset_key_bounds and propagate it through cloud/doris PB conversion and snapshot creation.
  • Teach BE writers/compaction/index builder to persist/carry forward rowset_key_bounds when per-segment bounds are omitted.
  • Update truncation logic and unit tests around segment/rowset key-bounds truncation and the new config gate.

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.

Show a summary per file
FileDescription
gensrc/proto/olap_file.protoAdds rowset_key_bounds to rowset meta protos.
cloud/src/meta-service/meta_service_job.cppStrips rowset_key_bounds when recycle-log stripping is enabled.
cloud/src/meta-service/meta_service.cppAccounts for rowset_key_bounds size when computing key-bounds bytes.
be/test/storage/segment/segments_key_bounds_truncation_test.cppExpands tests to cover config-gated rowset-level bounds and MoW behavior.
be/src/storage/task/index_builder.cppCopies either per-segment bounds or rowset-level bounds into output rowset meta.
be/src/storage/rowset/rowset_meta.hAdds rowset-level bounds accessors + changes first/last key bound getters.
be/src/storage/rowset/rowset_meta.cppAdds rowset-level bounds building/truncation + merge behavior updates.
be/src/storage/rowset/beta_rowset_writer.hTracks optional rowset-level bounds during rowset writer aggregation.
be/src/storage/rowset/beta_rowset_writer.cppPersists rowset-level bounds based on config / availability.
be/src/storage/compaction/compaction.cppPropagates rowset-level bounds when segment bounds are absent.
be/src/common/config.h / be/src/common/config.cppIntroduces enable_rowset_key_bounds_for_non_mow config (default false).
be/src/cloud/pb_convert.cppCopies/swaps rowset_key_bounds between doris/cloud PBs.
be/src/cloud/cloud_snapshot_mgr.cppCopies rowset_key_bounds when creating snapshot rowset meta.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +364 to +368
bool get_first_segment_key_bound(KeyBoundsPB* key_bounds) const {
if (_rowset_meta_pb.has_rowset_key_bounds()) {
*key_bounds = _rowset_meta_pb.rowset_key_bounds();
return true;
}

CopilotAIApr 13, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

get_first_segment_key_bound / get_last_segment_key_bound now return the full rowset_key_bounds when present. That changes the meaning of the returned KeyBoundsPB: the “first segment bound” will carry the rowset max key (not the first segment’s max), and the “last segment bound” will carry the rowset min key (not the last segment’s min). If any caller relies on both min/max being the segment’s range (not just one side), this will yield incorrect results. Consider either (a) keeping these APIs strictly “segment” by only setting the relevant side (first: min + first-segment max if available; last: last-segment min + max), or (b) introducing a separate get_rowset_key_bounds() and leaving the segment APIs to consult segments_key_bounds only.

Copilot uses AI. Check for mistakes.
Comment on lines +377 to +381
bool get_last_segment_key_bound(KeyBoundsPB* key_bounds) const {
if (_rowset_meta_pb.has_rowset_key_bounds()) {
*key_bounds = _rowset_meta_pb.rowset_key_bounds();
return true;
}

CopilotAIApr 13, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

get_first_segment_key_bound / get_last_segment_key_bound now return the full rowset_key_bounds when present. That changes the meaning of the returned KeyBoundsPB: the “first segment bound” will carry the rowset max key (not the first segment’s max), and the “last segment bound” will carry the rowset min key (not the last segment’s min). If any caller relies on both min/max being the segment’s range (not just one side), this will yield incorrect results. Consider either (a) keeping these APIs strictly “segment” by only setting the relevant side (first: min + first-segment max if available; last: last-segment min + max), or (b) introducing a separate get_rowset_key_bounds() and leaving the segment APIs to consult segments_key_bounds only.

Copilot uses AI. Check for mistakes.
Comment on lines 339 to +342
void RowsetMeta::set_segments_key_bounds(const std::vector<KeyBoundsPB>& segments_key_bounds) {
if (!segments_key_bounds.empty()) {
*_rowset_meta_pb.mutable_rowset_key_bounds() = build_rowset_key_bounds(segments_key_bounds);
}

CopilotAIApr 13, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This unconditionally persists rowset_key_bounds whenever set_segments_key_bounds() is called, including when config::enable_rowset_key_bounds_for_non_mow is false. The PR description states “non-MoW rowsets can persist rowset-level key bounds only when enable_rowset_key_bounds_for_non_mow is set to true”. If the intent is truly “only persist rowset_key_bounds behind the gate”, then this needs to be guarded (or moved to the gated write path). If the intent is “only omit per-segment bounds behind the gate (but always persist rowset-level bounds when available)”, then the PR description (and any related comments) should be updated to match the actual behavior.

Copilot uses AI. Check for mistakes.
Comment on lines +793 to +804
std::string first_key;
bool has_first_key = rowset->first_key(&first_key);
std::string last_key;
bool has_last_key = rowset->last_key(&last_key);
DCHECK_EQ(has_first_key, has_last_key);
if (has_first_key) {
if (!_rowset_key_bounds.has_value()) {
_rowset_key_bounds.emplace();
_rowset_key_bounds->set_min_key(first_key);
}
_rowset_key_bounds->set_max_key(last_key);
}

CopilotAIApr 13, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rowset->first_key() / rowset->last_key() may involve extra work (depending on implementation) even though the metadata paths now carry rowset_key_bounds / segments_key_bounds. Since you only need rowset-level min/max here, consider reading from rowset->rowset_meta() (e.g., rowset_meta->rowset_key_bounds() when present, or get_first_segment_key_bound/get_last_segment_key_bound) to avoid any potential segment/index reads during add_rowset().

Copilot uses AI. Check for mistakes.
Issue Number: None
Related PR: None
Problem Summary: Persist rowset-level key bounds for non-MoW rowsets so active rowset metadata no longer depends on per-segment key bounds for non-MoW read paths, while keeping full segment key bounds for MoW rowsets and preserving compatibility across compaction, snapshot, cloud conversion, and recycle flows.
None
- Test: Attempted BE unit test and targeted object compilation
- Unit Test / Manual test
- Behavior changed: Yes (non-MoW rowsets now persist rowset-level key bounds instead of full per-segment key bounds; MoW remains unchanged)
- Does this need documentation: No
@liaoxin01
liaoxin01force-pushed the liaoxin/non-mow-rowset-key-bounds-compat branch from b91d64c to 6c101beCompareApril 13, 2026 15:59
@liaoxin01

Copy link
Copy Markdown
ContributorAuthor

run buildall

@liaoxin01

Copy link
Copy Markdown
ContributorAuthor

run buildall

@hello-stephen

Copy link
Copy Markdown
Contributor

Cloud UT Coverage Report

Increment line coverage 60.00% (3/5) 🎉

Increment coverage report
Complete coverage report

CategoryCoverage
Function Coverage78.48% (1798/2291)
Line Coverage64.12% (32286/50352)
Region Coverage65.05% (16243/24969)
Branch Coverage55.58% (8682/15622)

@hello-stephen

Copy link
Copy Markdown
Contributor

BE UT Coverage Report

Increment line coverage 88.65% (125/141) 🎉

Increment coverage report
Complete coverage report

CategoryCoverage
Function Coverage53.04% (20141/37975)
Line Coverage36.60% (189518/517799)
Region Coverage32.87% (147196/447821)
Branch Coverage34.00% (64431/189491)

@hello-stephen

Copy link
Copy Markdown
Contributor

BE Regression && UT Coverage Report

Increment line coverage 90.78% (128/141) 🎉

Increment coverage report
Complete coverage report

CategoryCoverage
Function Coverage71.85% (26721/37189)
Line Coverage54.86% (283205/516215)
Region Coverage51.97% (234892/451953)
Branch Coverage53.35% (101404/190073)

@liaoxin01

liaoxin01 commented Apr 19, 2026

Copy link
Copy Markdown
ContributorAuthor

rewrited by:#62604

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@liaoxin01@Thearas@hello-stephen