github-actions[bot] commented on code in PR #68279:
URL: https://github.com/apache/doris/pull/68279#discussion_r4057083753
##########
be/src/storage/segment/segment_iterator.cpp:
##########
@@ -899,7 +899,15 @@ Status
SegmentIterator::_get_row_ranges_by_column_conditions() {
(!_opts.topn_filter_source_node_ids.empty() ||
!_opts.col_id_to_predicates.empty() ||
_opts.delete_condition_predicates->num_of_column_predicate() > 0 ||
!_common_expr_ctxs_push_down.empty())) {
- RowRanges condition_row_ranges =
RowRanges::create_single(_segment->num_rows());
+ // Start from the rows that are still alive instead of the whole
segment: the
+ // bitmap already carries the key range and index pruning, so anything
outside
+ // it cannot be pruned again. Starting from the whole segment would
make the
+ // zone map / bloom filter counters report rows that were pruned
earlier, e.g.
+ // a point lookup that already matched a single row reports the whole
segment
+ // as zone-map filtered.
+ RowRanges condition_row_ranges =
+
RowRanges::create_single(static_cast<int64_t>(_row_bitmap.minimum()),
+
static_cast<int64_t>(_row_bitmap.maximum()) + 1);
Review Comment:
The min/max range is only a dense envelope, not the set of surviving rows. A
reachable scan such as `k IN (1, 5000)` leaves a sparse `_row_bitmap`, but this
seeds `[1,5001)`; `_get_row_ranges_from_conditions()` then computes
`rows_bf_filtered`/`rows_stats_filtered` from that envelope before the exact
bitmap is applied at line 913. Any zone-map/Bloom ranges covering the gap are
charged as newly filtered even though those rows were already removed by the
key/index bitmap. The point-lookup test covers a contiguous one-row range, so
please preserve the bitmap's exact ranges (or compute these deltas against the
candidate bitmap) and add a disjoint-range counter test. Final row output is
unaffected, but the profile counters remain inaccurate for
multi-range/index-pruned scans.
##########
regression-test/suites/query_profile/point_lookup_zone_map_profile.groovy:
##########
@@ -0,0 +1,68 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+import org.apache.doris.regression.action.ProfileAction
+
+suite('point_lookup_zone_map_profile', 'nonConcurrent') {
+ sql 'DROP TABLE IF EXISTS point_lookup_zone_map_profile'
+ sql '''
+ CREATE TABLE point_lookup_zone_map_profile (
+ k INT NOT NULL,
+ v INT NOT NULL
+ ) DUPLICATE KEY(k)
+ DISTRIBUTED BY HASH(k) BUCKETS 1
+ PROPERTIES (
+ "replication_num" = "1",
+ "disable_auto_compaction" = "true"
+ )
+ '''
+ sql '''
+ INSERT INTO point_lookup_zone_map_profile
+ SELECT number, number FROM numbers("number" = "200000")
+ '''
+
+ sql 'set enable_profile=true'
+ sql 'set profile_level=2'
+
+ def profileAction = new ProfileAction(context)
+ def token = UUID.randomUUID().toString()
+
+ // The key range narrows the scan down to a single row, so the zone map has
+ // nothing left to prune and must not report the rest of the segment as
+ // filtered. The token only keeps the statement identifiable in the
profile.
+ sql """
+ SELECT COUNT(*) FROM point_lookup_zone_map_profile
+ WHERE k = 12345 AND v = 12345 AND '${token}' IS NOT NULL
+ """
+
+ def profile = profileAction
+ .getProfileBySql(token, ['RowsKeyRangeFiltered',
'RowsStatsFiltered'])
+ .toString()
+ logger.info("profile of ${token}: ${profile}")
+
+ def keyRangeFiltered = (profile =~
/RowsKeyRangeFiltered:\s*([^\n]+)/).collect { it[1].trim() }
+ def statsFiltered = (profile =~ /RowsStatsFiltered:\s*([^\n]+)/).collect {
it[1].trim() }
+ assertTrue(!keyRangeFiltered.isEmpty(), "no RowsKeyRangeFiltered in
profile: ${profile}")
+ assertTrue(!statsFiltered.isEmpty(), "no RowsStatsFiltered in profile:
${profile}")
+ assertTrue(keyRangeFiltered.any { it != '0' },
+ "the key range should have filtered rows: ${profile}")
+ statsFiltered.each {
+ assertEquals('0', it, "rows pruned by the key range must not be
counted again: ${profile}")
+ }
+
+ sql 'DROP TABLE IF EXISTS point_lookup_zone_map_profile'
Review Comment:
This suite already drops the table before creating it (line 21). Please
remove the trailing DROP here: the repository test convention is to preserve
successful-run tables for post-failure/profile debugging, while a failing
assertion would skip this cleanup anyway.
--
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]
