github-actions[bot] commented on code in PR #66338:
URL: https://github.com/apache/doris/pull/66338#discussion_r3827003190
##########
be/src/storage/rowset/beta_rowset_writer.cpp:
##########
@@ -1074,6 +1074,17 @@ Status
BaseBetaRowsetWriter::_build_rowset_meta(RowsetMeta* rowset_meta, bool ch
!is_segment_overlapping(segments_encoded_key_bounds) &&
_context.tablet_schema->cluster_key_uids().empty()) {
rowset_meta->set_segments_overlap(NONOVERLAPPING);
+ } else if (!segments_encoded_key_bounds.empty() &&
+ is_segment_overlapping(segments_encoded_key_bounds) &&
Review Comment:
[P2] Classify overlap before truncating key bounds
`segments_encoded_key_bounds` here already comes from
`_segid_statistics_map`, whose `add_segment` path truncates min/max encodings
to 36 bytes by default. Two disjoint string-key segments with a long common
prefix therefore compare as `cur_min <= last`, and this new branch changes the
compaction output from `NONOVERLAPPING` to `OVERLAPPING`. Every later ordered
read pays for a merge heap (vertical reads eagerly initialize all segments),
and `get_compaction_score()` becomes the segment count, which can trigger
needless binlog rewrites. Please retain the exact overlap fact before
truncation and persist only the bounds in truncated form, and cover disjoint
long-prefix keys.
##########
be/src/storage/merger.cpp:
##########
@@ -77,6 +77,12 @@ Status Merger::vmerge_rowsets(BaseTabletSPtr tablet,
ReaderType reader_type,
reader_params.tablet = tablet;
reader_params.reader_type = reader_type;
reader_params.read_row_binlog = tablet->is_row_binlog_tablet();
+ if (reader_params.read_row_binlog) {
+ // Row-binlog horizontal (non-vertical) compaction must produce a
globally
+ // (key, TSO)-ordered output.
+ reader_params.read_orderby_key = true;
Review Comment:
[P1] Preserve ordering for pre-upgrade horizontal outputs
This orders rowsets only when horizontal compaction rebuilds them after the
upgrade. A BE running the base code with `enable_vertical_compaction=false` can
already have persisted a multi-segment row-binlog output as `NONOVERLAPPING`
even though its physical ranges are `1..N@TSO1, 1..N@TSO2`; an old BE can also
produce one during rolling upgrade. The base reader's `force_key_ordered_read`
arm was the read-side safety net for such segments, but this PR removes it, so
a new reader unions the stale rowset and MIN_DELTA can close a key before its
later event. Please retain a compatibility fallback or migrate/recompute
existing metadata, with an upgrade-style stale-`NONOVERLAPPING` test. This is
distinct from r3820374567: that thread fixed future production on upgraded BEs,
while this concerns existing or concurrently old-produced rowsets.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
