github-actions[bot] commented on code in PR #67019:
URL: https://github.com/apache/doris/pull/67019#discussion_r3861405425
##########
be/src/storage/segment/segment_iterator.cpp:
##########
@@ -3350,48 +3386,36 @@ Status
SegmentIterator::_apply_expr_zonemap_to_row_ranges(const VExprContextSPtr
continue;
}
const auto* tablet_column = _schema->column(cid);
- std::shared_ptr<ColumnReader> reader;
- Status st =
- _segment->get_column_reader(*tablet_column, &reader,
_opts.stats, &_opts.io_ctx);
- if (st.is<ErrorCode::NOT_FOUND>()) {
- continue;
- }
- RETURN_IF_ERROR(st);
- if (reader == nullptr || !reader->has_zone_map()) {
+ if (tablet_column == nullptr) {
continue;
}
- const std::vector<ZoneMapPB>* page_zone_maps = nullptr;
- RETURN_IF_ERROR(reader->get_page_zone_maps(iter_opts,
&page_zone_maps));
- if (page_zone_maps == nullptr || page_zone_maps->empty()) {
+ auto data_type = _segment->get_data_type_of(*tablet_column,
_schema->data_type(cid), _opts);
+ if (data_type == nullptr) {
continue;
}
- auto data_type = _segment->get_data_type_of(*tablet_column,
_schema->data_type(cid), _opts);
- RowRanges column_ranges;
ZoneMapEvalStats page_stats;
- for (uint32_t page_index = 0; page_index < page_zone_maps->size();
++page_index) {
- RowRange page_range;
- RETURN_IF_ERROR(reader->get_row_range_for_page(page_index,
iter_opts, &page_range));
- if (!page_range.is_valid() || page_range.to() <= min_rowid) {
- continue;
- }
- ZoneMapEvalContext ctx;
- ZoneMapEvalContext::SlotZoneMap slot_zone_map;
- slot_zone_map.data_type = data_type;
- ZoneMap zone_map;
- RETURN_IF_ERROR(
- ZoneMap::from_proto((*page_zone_maps)[page_index],
data_type, zone_map));
- slot_zone_map.zone_map =
std::make_shared<ZoneMap>(std::move(zone_map));
- ctx.slots.emplace(slot_index, std::move(slot_zone_map));
- const auto result =
VExprContext::evaluate_zonemap_filter(slot_conjuncts, ctx);
- page_stats.merge_page_eval_stats(ctx.stats);
- if (result != ZoneMapFilterResult::kNoMatch) {
- column_ranges.add(
- RowRange(std::max<int64_t>(page_range.from(),
min_rowid), page_range.to()));
- } else {
- ++_opts.stats->expr_zonemap_filtered_pages;
- }
- }
+ // One page proving a conjunct always true says nothing about the
other pages, so nothing
+ // can be dropped here. Reused across pages so the walk does not
allocate per page.
+ std::vector<bool> ignored_always_true;
+ RowRanges column_ranges = RowRanges::create_single(num_rows());
+ RETURN_IF_ERROR(prune_pages_by_zone_map(
+ _column_iterators[cid].get(), min_rowid,
+ [&](const ZoneMap& zone_map) {
+ ZoneMapEvalContext ctx;
+ ZoneMapEvalContext::SlotZoneMap slot_zone_map;
+ slot_zone_map.data_type = data_type;
+ slot_zone_map.zone_map =
std::make_shared<ZoneMap>(zone_map);
Review Comment:
[P2] Preserve ownership when passing decoded page zone maps to the
expression evaluator. The old loop moved the local `ZoneMap` into this shared
pointer, but the shared walker now passes it as `const&`, so this line
deep-copies both `Field` bounds for every page and referenced slot. For string
columns that adds up to two 512-byte buffer copies/allocations per page on the
pruning hot path. Please provide an owning/rvalue callback for the expression
consumer (predicate judges can still bind a const reference), or otherwise
retain the former move behavior.
##########
be/src/storage/segment/segment.cpp:
##########
@@ -364,16 +363,33 @@ Status Segment::_open_index_file_reader() {
return Status::OK();
}
-bool Segment::is_tso_placeholder_col(int cid, const ReadSchema& schema,
- const StorageReadOptions& read_options)
const {
- if (read_options.version.first != read_options.version.second) {
- return false;
+std::optional<Field> Segment::_read_time_const_value(int32_t col_uid,
+ const StorageReadOptions&
read_options) const {
+ // Three columns read as a value the file does not hold.
__DORIS_COMMIT_TSO_COL__ and
+ // __DORIS_BINLOG_TSO__ both read as the rowset's commit_tso, and
__DORIS_VERSION_COL__ as its
+ // version.
+ if (col_uid < 0 || read_options.version.second <= 0 ||
Review Comment:
[P1] Propagate the resolved Rowset context into direct row-ID reads. This
helper returns no synthetic value when `StorageReadOptions.version` is left at
its default, but `PointQueryExecutor` and `RowIDFetcher` both call
`seek_and_read_by_rowid` with only stats/I/O even though they already resolved
the exact Rowset. A projection of `__DORIS_VERSION_COL__` (and COMMIT_TSO when
applicable) therefore still reads the on-disk 0 placeholder instead of the
value used by the normal `SegmentIterator` path. Please populate `version` and
`commit_tso` for those callers, or make the row-ID API require that context,
and cover both direct paths with hidden-column projection tests.
--
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]
