github-actions[bot] commented on code in PR #67360:
URL: https://github.com/apache/doris/pull/67360#discussion_r3901772345
##########
be/src/storage/merger.cpp:
##########
@@ -78,6 +94,10 @@ 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();
+ reader_params.runtime_state = runtime_state;
Review Comment:
[P1] Preserve compaction's disposable cache policy
This assignment does more than propagate cancellation. `BetaRowsetReader`
first marks every non-query read as disposable, but the mere presence of a
`RuntimeState` then overwrites both `read_file_cache` and `is_disposable` from
query options. A default state leaves `enable_file_cache` and
`disable_file_cache` false, so an uncancelled compaction using this parameter
changes DISPOSABLE traffic to the NORMAL cache queue (and changes the
read-cache flag) just because cancellation was enabled. Please restrict those
query-option overrides to `READER_QUERY`, or pass cancellation through a
channel that preserves compaction I/O policy; the current pre-cancelled test
never opens a reader and cannot detect this.
##########
be/src/storage/merger.h:
##########
@@ -70,13 +79,17 @@ class Merger {
BaseTabletSPtr tablet, ReaderType reader_type, const TabletSchema&
cur_tablet_schema,
const std::vector<RowsetReaderSharedPtr>& src_rowset_readers,
RowsetWriter* dst_rowset_writer, Statistics* stats_output,
- std::optional<std::pair<int64_t, int64_t>> segment_range =
std::nullopt);
+ std::optional<std::pair<int64_t, int64_t>> segment_range =
std::nullopt,
+ std::optional<KeyRange> key_range = std::nullopt,
+ RuntimeState* runtime_state = nullptr);
static Status vertical_merge_rowsets(
BaseTabletSPtr tablet, ReaderType reader_type, const TabletSchema&
tablet_schema,
const std::vector<RowsetReaderSharedPtr>& src_rowset_readers,
RowsetWriter* dst_rowset_writer, uint32_t max_rows_per_segment,
int64_t merge_way_num,
Statistics* stats_output, VerticalCompactionProgressCallback
progress_cb = nullptr,
- std::optional<std::pair<int64_t, int64_t>> segment_range =
std::nullopt);
+ std::optional<std::pair<int64_t, int64_t>> segment_range =
std::nullopt,
+ std::optional<KeyRange> key_range = std::nullopt,
Review Comment:
[P1] Enforce the physical key-range contract at this boundary
This new API accepts ranges that the downstream seek path cannot implement
correctly. `SegmentIterator::_lazy_init` skips key pruning for Z-order and
cluster-key layouts, so two range subtasks on either layout both emit the full
selected segments. It also accepts empty, no-key, and value-inclusive tuples:
`TabletReader` permits widths through `num_columns`, and short-key lookup then
binary-compares value columns that are not ordered for a DUP-key tablet. Please
reject a range unless the layout supports ordered key seeking and each present
bound is a nonempty leading-key prefix (`width <= num_key_columns`), or
implement and test the missing physical-order seek semantics.
##########
be/src/storage/merger.cpp:
##########
@@ -268,7 +292,11 @@ Status Merger::vertical_compact_one_group(
reader_params.tablet = tablet;
reader_params.reader_type = reader_type;
reader_params.read_row_binlog = tablet->is_row_binlog_tablet();
+ reader_params.runtime_state = runtime_state;
Review Comment:
[P2] Poll cancellation while initializing vertical readers
After the group-level check, `reader.init()` walks every input rowset in
`VerticalBlockReader::_get_segment_iterators`, calling `init()` and
`get_segment_iterators()` without consulting this state; the next poll is only
after the whole initialization returns. Cancellation arriving during a large
remote compaction therefore continues loading all remaining rowsets and
segments. The horizontal `BlockReader` already checks once per rowset. Please
mirror that check in the vertical initialization loop and add a synchronized
mid-initialization cancellation test rather than only the pre-cancelled case.
##########
be/src/storage/segment/segment_iterator.cpp:
##########
@@ -710,8 +710,11 @@ Status SegmentIterator::_get_row_ranges_by_keys() {
return Status::OK();
}
- // Read & seek key columns is a waste of time when no key column in _schema
- if (std::none_of(_schema->columns().begin(), _schema->columns().end(),
+ // Read & seek key columns is a waste of time when no key column in
_schema.
+ // However, parallel base compaction uses key ranges to split tasks, so
value-only vertical
+ // groups must still apply the key range to read the same physical rows as
the key group.
+ if (_opts.io_ctx.reader_type != ReaderType::READER_BASE_COMPACTION &&
Review Comment:
[P1] Keep every vertical column group on the same physical range
The new `key_range` parameter is accepted for every `ReaderType`, but this
condition makes only base-compaction value groups bypass the no-key fast path.
For a vertical cumulative, full, or alter-table merge, the key group contains
key columns and filters to, for example, physical rows 2-6, while a value-only
group returns here and starts at row 0. `VerticalMaskMergeIterator` then
consumes the key group's five source entries against values from rows 0-4 and
releases the remaining suffix, silently writing mismatched columns. Please
either apply a nonempty range to value-only groups for every supported reader
type or reject unsupported reader types at the Merger boundary, with a non-base
vertical 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]
