github-actions[bot] commented on code in PR #66338:
URL: https://github.com/apache/doris/pull/66338#discussion_r3820374567
##########
be/src/storage/rowset/beta_rowset_reader.h:
##########
@@ -61,8 +61,7 @@ class BetaRowsetReader : public RowsetReader {
bool is_merge_iterator() const override {
return _read_context->need_ordered_result && _get_segment_num() > 1 &&
- (_rowset->rowset_meta()->is_segments_overlapping() ||
- _read_context->force_key_ordered_read);
+ _rowset->rowset_meta()->is_segments_overlapping();
Review Comment:
[P1] Preserve ordering for non-vertical row-binlog compaction
With `enable_vertical_compaction=false`, `Merger::vmerge_rowsets` sets
`read_row_binlog` but no ordered-read flag, so `VCollectIterator` disables its
outer merge and the compaction writer can append two input ranges as
`1..N@TSO1, 1..N@TSO2`. Normal compaction nevertheless initializes the output
as `NONOVERLAPPING`, and `_build_rowset_meta` never changes that value back
when bounds overlap. This overlap-only predicate then chooses a segment union
(`1..N,1..N`), so MIN_DELTA closes the first key group before its later event
and returns the wrong delta. Please make horizontal row-binlog compaction
perform a real `(key, TSO)` merge (and publish accurate overlap metadata), and
cover `enable_vertical_compaction=false` with overlapping input keys and a
forced segment boundary. This is distinct from the existing direct-load
coverage thread: that rowset is correctly marked overlapping; this producer is
not.
##########
be/src/storage/segment/segment_iterator.cpp:
##########
@@ -1610,6 +1610,15 @@ bool SegmentIterator::_need_read_data(ColumnId cid) {
if (_has_delete_predicate(cid)) {
return true;
}
+ // Row-binlog incremental reads force-push the TSO range predicate (see
+ // OlapScanner::_init_tso_pushdown), and the merge iterator uses the TSO
column as its
+ // sequence sort key (BetaRowsetReader sets binlog_tso_idx). On a
cross-version rowset
+ // (e.g. produced by binlog LMax quick-merge) the TSO zonemap can be
always-true, so the
+ // pruning below would skip reading it and fill placeholder zeros,
breaking the merge
+ // ordering. The TSO column carries real values on disk, so force it to be
read.
+ if (_opts.read_row_binlog && cid ==
_opts.tablet_schema->binlog_tso_col_idx()) {
Review Comment:
[P2] Avoid forcing TSO I/O for unordered row-binlog scans
This condition applies to every `read_row_binlog` scan, including
APPEND_ONLY/raw reads that neither project TSO nor build a TSO merge heap. For
a cross-version rowset whose predicate-only TSO range is proven always true by
the zonemap, the existing logic below can skip that unprojected column; this
early return now reads and decompresses every TSO page even though no consumer
uses it. Please gate the override on the ordered MIN_DELTA/DETAIL merge
requirement (or equivalent `binlog_tso_idx` state), and add a read-statistics
test for the always-true APPEND_ONLY case while retaining the quick-merge
ordering test.
--
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]
