Copilot commented on code in PR #64165:
URL: https://github.com/apache/doris/pull/64165#discussion_r3366553341
##########
be/src/storage/segment/segment_iterator.cpp:
##########
@@ -1332,6 +1332,7 @@ Status SegmentIterator::_apply_index_expr() {
_opts.runtime_state->query_options().enable_ann_index_result_cache;
for (const auto& expr_ctx : _common_expr_ctxs_push_down) {
+ const size_t origin_rows = _row_bitmap.cardinality();
if (Status st = expr_ctx->evaluate_inverted_index(num_rows());
!st.ok()) {
if (_downgrade_without_index(st) || st.code() ==
ErrorCode::NOT_IMPLEMENTED_ERROR) {
continue;
Review Comment:
Per-filter `INDEX_INVERTED` stats for common-expr inverted index evaluation
are not recorded incrementally: `_row_bitmap` is not updated inside this loop,
so each filter sees the same `origin_rows` and the `output_rows` reflect
applying only that single bitmap rather than the cumulative AND of previous
expr bitmaps. This makes the per-filter input/output/filtered rows misleading
and contradicts the goal of order-dependent (incremental) filtering counters.
##########
be/src/exprs/vexpr_context.cpp:
##########
@@ -230,14 +234,28 @@ Status VExprContext::execute_filter(const Block* block,
uint8_t* __restrict resu
Status VExprContext::execute_conjuncts(const VExprContextSPtrs& ctxs,
const std::vector<IColumn::Filter*>*
filters,
bool accept_null, const Block* block,
- IColumn::Filter* result_filter, bool*
can_filter_all) {
+ IColumn::Filter* result_filter, bool*
can_filter_all,
+ std::optional<ScanFilterStage>
scan_filter_stage) {
size_t rows = block->rows();
DCHECK_EQ(result_filter->size(), rows);
*can_filter_all = false;
auto* __restrict result_filter_data = result_filter->data();
for (const auto& ctx : ctxs) {
+ const int64_t input_rows =
+ scan_filter_stage.has_value()
+ ? std::count(result_filter_data, result_filter_data +
rows, 1)
+ : 0;
Review Comment:
`execute_conjuncts()` computes `std::count()` for every conjunct whenever
`scan_filter_stage` is provided, even if the current conjunct has no
`scan_filter_handle()`. Call sites now pass a stage unconditionally (e.g.
residual/common expr), so with `profile_level<=0` this still adds an O(rows *
conjuncts) hot-path cost while recording nothing. Gate the row
counting/recording on `ctx->scan_filter_handle()` to keep the non-profile fast
path cheap.
##########
be/src/exec/operator/olap_scan_operator.cpp:
##########
@@ -1191,6 +1261,14 @@ Status
OlapScanLocalState::_build_key_ranges_and_filters() {
for (const auto& it : _slot_id_to_predicates[*key_to_erase]) {
if (!can_erase_predicate(*it)) {
new_predicates.push_back(it);
+ } else if (_scan_filter_profile != nullptr) {
+ const auto& handle = it->scan_filter_handle();
+ DORIS_CHECK(handle);
+ auto& source_ids =
_slot_id_to_scan_filter_ids_for_key_range[*key_to_erase];
+ if (std::find(source_ids.begin(), source_ids.end(),
handle.filter_id) ==
+ source_ids.end()) {
+ source_ids.push_back(handle.filter_id);
+ }
}
Review Comment:
`DORIS_CHECK(handle)` here can crash queries when scan-filter profiling is
enabled if any predicate was subsumed into key ranges but did not get a
`ScanFilterHandle` attached (e.g. due to an unexpected normalization path).
Since this is only for lineage/profile display, it should not be a hard
invariant in release builds; skip recording the source id when the handle is
missing.
##########
be/src/storage/segment/column_reader.cpp:
##########
@@ -515,18 +528,28 @@ bool ColumnReader::_zone_map_match_condition(const
ZoneMap& zone_map,
Status ColumnReader::_get_filtered_pages(
const AndBlockColumnPredicate* col_predicates,
const std::vector<std::shared_ptr<const ColumnPredicate>>*
delete_predicates,
- std::vector<uint32_t>* page_indexes, const ColumnIteratorOptions&
iter_opts) {
+ const RowRanges& input_row_ranges, std::vector<uint32_t>* page_indexes,
+ const ColumnIteratorOptions& iter_opts) {
RETURN_IF_ERROR(_load_zone_map_index(_use_index_page_cache,
_opts.kept_in_memory, iter_opts));
+ RETURN_IF_ERROR(_load_ordinal_index(_use_index_page_cache,
_opts.kept_in_memory, iter_opts));
const std::vector<ZoneMapPB>& zone_maps =
_zone_map_index->page_zone_maps();
- size_t page_size = _zone_map_index->num_pages();
- for (size_t i = 0; i < page_size; ++i) {
+ int page_size = cast_set<int>(_zone_map_index->num_pages());
+ for (int i = 0; i < page_size; ++i) {
+ const ordinal_t page_first_id = _ordinal_index->get_first_ordinal(i);
+ const ordinal_t page_last_id = _ordinal_index->get_last_ordinal(i) + 1;
+ const int64_t page_input_rows =
+ row_ranges_intersection_count(input_row_ranges, page_first_id,
page_last_id);
+ if (page_input_rows == 0) {
+ continue;
+ }
if (zone_maps[i].pass_all()) {
page_indexes->push_back(cast_set<uint32_t>(i));
} else {
Review Comment:
When a page zone-map is marked `pass_all`, the page is accepted but no
per-filter stats are recorded for `INDEX_ZONE_MAP`. This makes the stage look
like it was “not applied” and undercounts `input_rows/output_rows` for filters
that did participate but had zero pruning effect. Record a pass-through
(input==output) for each predicate handle on `pass_all` pages so stage
participation and row totals stay accurate.
##########
be/src/storage/segment/column_reader.cpp:
##########
@@ -591,9 +614,16 @@ Status ColumnReader::get_row_ranges_by_bloom_filter(const
AndBlockColumnPredicat
for (auto& pid : page_ids) {
std::unique_ptr<BloomFilter> bf;
RETURN_IF_ERROR(bf_iter->read_bloom_filter(pid, &bf));
- if (col_predicates->evaluate_and(bf.get())) {
- bf_row_ranges.add(RowRange(_ordinal_index->get_first_ordinal(pid),
- _ordinal_index->get_last_ordinal(pid) +
1));
+ const ordinal_t page_first_id = _ordinal_index->get_first_ordinal(pid);
+ const ordinal_t page_last_id = _ordinal_index->get_last_ordinal(pid) +
1;
+ const int64_t page_input_rows =
+ row_ranges_intersection_count(*row_ranges, page_first_id,
page_last_id);
+ if (page_input_rows == 0) {
+ continue;
+ }
+ if (col_predicates->evaluate_and_with_scan_filter(
+ bf.get(), ScanFilterStage::INDEX_BLOOM_FILTER,
page_input_rows)) {
Review Comment:
`get_row_ranges_by_bloom_filter()` now computes `page_input_rows` by
intersecting the current `RowRanges` with every candidate page range, even when
no predicates have a `ScanFilterHandle` (i.e. scan-filter profiling disabled).
This adds extra work in the bloom-filter index loop without producing any
stats. Similar to zone map, this should be guarded behind an explicit
profiling-enabled flag or an efficient “has scan-filter handles” check.
##########
be/src/storage/segment/column_reader.cpp:
##########
@@ -515,18 +528,28 @@ bool ColumnReader::_zone_map_match_condition(const
ZoneMap& zone_map,
Status ColumnReader::_get_filtered_pages(
const AndBlockColumnPredicate* col_predicates,
const std::vector<std::shared_ptr<const ColumnPredicate>>*
delete_predicates,
- std::vector<uint32_t>* page_indexes, const ColumnIteratorOptions&
iter_opts) {
+ const RowRanges& input_row_ranges, std::vector<uint32_t>* page_indexes,
+ const ColumnIteratorOptions& iter_opts) {
RETURN_IF_ERROR(_load_zone_map_index(_use_index_page_cache,
_opts.kept_in_memory, iter_opts));
+ RETURN_IF_ERROR(_load_ordinal_index(_use_index_page_cache,
_opts.kept_in_memory, iter_opts));
const std::vector<ZoneMapPB>& zone_maps =
_zone_map_index->page_zone_maps();
- size_t page_size = _zone_map_index->num_pages();
- for (size_t i = 0; i < page_size; ++i) {
+ int page_size = cast_set<int>(_zone_map_index->num_pages());
+ for (int i = 0; i < page_size; ++i) {
+ const ordinal_t page_first_id = _ordinal_index->get_first_ordinal(i);
+ const ordinal_t page_last_id = _ordinal_index->get_last_ordinal(i) + 1;
+ const int64_t page_input_rows =
+ row_ranges_intersection_count(input_row_ranges, page_first_id,
page_last_id);
+ if (page_input_rows == 0) {
Review Comment:
`_get_filtered_pages()` now computes `page_input_rows` via
`row_ranges_intersection_count()` for every page and calls
`evaluate_and_with_scan_filter()`. When scan-filter profiling is disabled,
predicates have empty `ScanFilterHandle`s, so this becomes pure overhead on a
very hot path (zone map pruning) without producing any profile stats. Consider
plumbing a cheap `collect_scan_filter_stats` flag (or otherwise detecting
presence of scan-filter handles once) so the old fast path is preserved when
scan filter profiling is off.
--
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]
