github-actions[bot] commented on code in PR #66338:
URL: https://github.com/apache/doris/pull/66338#discussion_r3711013645
##########
be/src/storage/iterator/vcollect_iterator.cpp:
##########
@@ -501,8 +501,16 @@ Status VCollectIterator::Level0Iterator::init(bool
get_data_by_ref) {
// }
// so first child load first row and other child row_pos = -1
void VCollectIterator::Level0Iterator::init_for_union(bool get_data_by_ref) {
+ // The union path (_merge == false) always reads whole blocks via
next(Block*)
+ // or ensure_first_row_ref(), both of which require _get_data_by_ref ==
false.
+ // get_data_by_ref only makes sense on the merge path, where init()
prefetches
+ // the block view and the child is consumed row by row through the merge
heap.
+ // Honoring it here would wrongly put a multi-segment child
+ // (is_merge_iterator() == true, e.g. a table-stream scan forcing
+ // force_key_ordered_read) into by-reference mode and trip
+ // CHECK(!_get_data_by_ref). See issue #65901.
_is_merge_iterator = _rs_reader->is_merge_iterator();
- _get_data_by_ref = get_data_by_ref && _is_merge_iterator;
+ _get_data_by_ref = false;
Review Comment:
[P1] Initialize later block-mode union children before row-wise handoff
This forces every union child into block mode, but `init_for_union()` still
does not allocate `_block` or initialize `_ref` for children after the first
(only the first non-empty child reaches `ensure_first_row_ref()`). When a
row-wise consumer such as MIN_DELTA/DETAIL advances to a later multi-segment
child, `next(IteratorRowRef*)` takes the new false branch and evaluates
`_block->rows()` before `refresh_current_row()` can lazily allocate it, so a
pair of key-disjoint multi-segment rowsets can null-dereference at child
handoff. Please establish a valid empty block/pre-first-row state for every
child entering block mode (resetting cursors alone is insufficient), and cover
a row-wise union consumer as well as APPEND_ONLY.
##########
be/src/storage/tablet/tablet_reader.cpp:
##########
@@ -196,7 +196,12 @@ Status TabletReader::_capture_rs_readers(const
ReaderParams& read_params) {
_reader_context.stats = &_stats;
_reader_context.use_page_cache = read_params.use_page_cache;
_reader_context.sequence_id_idx = _sequence_col_idx;
- _reader_context.is_unique = tablet()->keys_type() == UNIQUE_KEYS;
+ // Derive is_unique from the read schema, not the raw tablet. For binlog /
+ // table-stream reads the read schema is the row-binlog schema (DUP_KEYS),
where
+ // every row is an independent change event that must not be deduplicated
by user
+ // key; using tablet()->keys_type() (UNIQUE_KEYS for MoW/UNIQUE tables)
would make
+ // the rowset merge iterator drop events across segments. Mirrors
VerticalBlockReader.
+ _reader_context.is_unique = _tablet_schema->keys_type() == UNIQUE_KEYS;
Review Comment:
[P2] Add coverage for equal keys across segments of one rowset
This flag is consumed by `BetaRowsetReader` only when its per-rowset
`VMergeIterator` compares segments. Every new load here is internally
key-unique; Section C overlaps two separate rowsets/Tsos, so the outer
`VCollectIterator` handles it and the suite still passes if only this line is
reverted. The older raw `binlog()` multi-segment test also normally uses scan
type NONE and does not force this internal merge. Please add one table-stream
load whose same key/TSO events cross a forced segment boundary and assert the
retained event chain, so the event-loss behavior fixed by this line has a
direct regression.
--
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]
