github-actions[bot] commented on code in PR #66145:
URL: https://github.com/apache/doris/pull/66145#discussion_r3688902273
##########
be/src/storage/segment/segment.cpp:
##########
@@ -379,6 +393,25 @@ bool Segment::is_tso_placeholder_col(int cid, const
Schema& schema,
return cid == schema.tso_col_idx();
}
+std::optional<Field> Segment::get_read_time_constant_value(
+ int cid, const Schema& schema, const StorageReadOptions& read_options)
const {
+ if (read_options.version.first != read_options.version.second) {
+ return std::nullopt;
+ }
+ if (cid == schema.version_col_idx()) {
Review Comment:
[P1] Use the logical VERSION_COL value on the normalized-predicate path
This helper is only consulted by expression ZoneMap pruning. When the
runtime filter is ready during scan open, `_normalize_predicate()` turns it
into `col_id_to_predicates`; `Segment::new_iterator()` and
`_get_row_ranges_from_conditions()` still test those predicates against the
physical VERSION value `[0,0]` before `_replace_version_col_if_needed()`
substitutes the rowset version. Thus a ready `VERSION_COL = 7` filter can
create an empty iterator for a version-7 rowset, while the forced-late
regression passes. Please use the logical constant for ordinary segment/page
predicate pruning too and add a ready-at-open case.
##########
be/src/storage/segment/segment_iterator.cpp:
##########
@@ -3297,22 +3438,127 @@ Status
SegmentIterator::current_block_row_locations(std::vector<RowLocation>* bl
return Status::OK();
}
+bool SegmentIterator::_has_arrived_late_runtime_filter() const {
+ return _late_runtime_filter_container != nullptr &&
+
_late_runtime_filter_container->arrived_cnt.load(std::memory_order_relaxed) > 0;
+}
+
+Status SegmentIterator::_install_late_runtime_filter(const VExprContextSPtrs&
expr_group,
+ VExprContextSPtrs*
installed_contexts) {
+ DORIS_CHECK(installed_contexts != nullptr);
+ DORIS_CHECK(!expr_group.empty());
+
+ VExprContextSPtrs local_contexts;
+ local_contexts.reserve(expr_group.size());
+ for (const auto& base_context : expr_group) {
+ DORIS_CHECK(base_context != nullptr);
+ VExprContextSPtr local_context;
+ RETURN_IF_ERROR(base_context->clone(_opts.runtime_state,
local_context));
Review Comment:
[P1] Do not reopen a shared expression tree from parallel iterators
Every iterator clones the same published base context, but
`VExprContext::clone()` shares `_root` and calls `_root->open()` again.
Parallel scanners/segments therefore write the same non-atomic expression
fields (`_open_finished`, and for IN predicates `_is_args_all_constant`), and a
later clone can do so while another iterator or the scanner residual executes
that tree. This is a C++ data race despite the safe pointer publication. Please
give each iterator an independently owned expression tree (or otherwise make
all root state immutable/context-owned); serializing only simultaneous clone
calls would not prevent a later reopen from racing with execution.
##########
be/src/storage/segment/segment_iterator.cpp:
##########
@@ -1176,6 +1264,21 @@ Status
SegmentIterator::_get_row_ranges_from_conditions(RowRanges* condition_row
_opts.stats->rows_stats_filtered +=
(pre_expr_zonemap_size - condition_row_ranges->count());
}
+ if (!_late_runtime_filter_ctxs.empty() &&
!condition_row_ranges->is_empty()) {
+ const auto rows_before_zonemap = condition_row_ranges->count();
+
RETURN_IF_ERROR(_apply_expr_zonemap_to_row_ranges(_late_runtime_filter_ctxs, 0,
+
condition_row_ranges));
+ const auto rows_after_zonemap = condition_row_ranges->count();
+ DORIS_CHECK_LE(rows_after_zonemap, rows_before_zonemap);
+ // This is a coarse informational count based on the condition
ranges. It may include
+ // rows already removed from _row_bitmap by other indexes, or rows
later excluded by
+ // delete bitmaps and parallel-scan ranges, so it can overestimate
the rows this
+ // iterator would actually read. The direct delta avoids extra
range and bitmap
+ // operations in the scan hot path while still reflecting late RF
ZoneMap pruning.
+ const auto rows_filtered = rows_before_zonemap -
rows_after_zonemap;
+ _opts.stats->rows_stats_filtered += rows_filtered;
+ _opts.stats->rows_late_runtime_filter_zonemap_filtered +=
rows_filtered;
Review Comment:
[P2] Keep the late-ZoneMap row counter tied to real candidates
`rows_filtered` is computed before the condition ranges are intersected with
this iterator's assigned `_row_bitmap` and before its delete bitmap is applied.
The counter therefore increases even when the late filter removes no row this
iterator could return; both new overlap tests intentionally expect 2,048 in
exactly those cases. Because this is exported as
`RowsLateRuntimeFilterZoneMapFiltered` and documented as incrementally pruned
rows, profiles can report storage-pruning benefit where none occurred. Please
count the cardinality removed from the actual candidate bitmap, or rename and
document this as estimated page-range coverage instead.
--
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]
