github-actions[bot] commented on code in PR #65471:
URL: https://github.com/apache/doris/pull/65471#discussion_r3563119021
##########
be/src/format_v2/table_reader.cpp:
##########
@@ -790,6 +790,15 @@ Status
TableReader::_evaluate_partition_prune_conjuncts(const VExprContextSPtrs&
for (const auto& conjunct : conjuncts) {
DORIS_CHECK(conjunct != nullptr);
DORIS_CHECK(conjunct->root() != nullptr);
+ const auto root = conjunct->root();
+ const auto impl = root->get_impl();
+ const auto predicate = impl != nullptr ? impl : root;
+ // Split pruning evaluates a predicate once before any file rows are
read. Reordering
+ // non-deterministic or error-preserving expressions can change their
row-level semantics,
+ // even when every referenced slot is a partition column.
+ if (!predicate->is_safe_to_execute_on_selected_rows()) {
+ continue;
Review Comment:
The `continue` here still changes evaluation order when there are multiple
conjuncts. `_build_table_conjuncts()` preserves `_conjuncts` order, and the
normal `execute_conjuncts()` path evaluates each context in order and only
stops once a context filters all rows. With this loop, an earlier unsafe
predicate is dropped but a later safe partition predicate can still be added to
`partition_conjuncts` and prune the split before the unsafe predicate ever
reaches the row-level `Scanner::_filter_output_block()` path. For example, on a
`part=7` split, `[assert_true(part != 7), part = 0]` should raise from the
first conjunct in the normal path, but this code skips it and lets `part = 0`
prune the file. Once a conjunct is not safe to move, split pruning should stop
considering later conjuncts for that split, or disable this pruning pass
entirely, rather than just skipping that one predicate.
##########
be/src/format_v2/table_reader.cpp:
##########
@@ -790,6 +790,15 @@ Status
TableReader::_evaluate_partition_prune_conjuncts(const VExprContextSPtrs&
for (const auto& conjunct : conjuncts) {
DORIS_CHECK(conjunct != nullptr);
DORIS_CHECK(conjunct->root() != nullptr);
+ const auto root = conjunct->root();
+ const auto impl = root->get_impl();
+ const auto predicate = impl != nullptr ? impl : root;
+ // Split pruning evaluates a predicate once before any file rows are
read. Reordering
+ // non-deterministic or error-preserving expressions can change their
row-level semantics,
+ // even when every referenced slot is a partition column.
+ if (!predicate->is_safe_to_execute_on_selected_rows()) {
Review Comment:
This guard also needs to cover the other split-level pre-execution path.
After `open_reader()` builds mappings, `_evaluate_constant_filters()` still
executes any table filter whose referenced entries are constants, and partition
columns become constant filter entries. It only skips runtime-filter wrappers;
it does not check the unwrapped predicate's
`is_safe_to_execute_on_selected_rows()` contract. A non-runtime conjunct such
as `part = 7 OR random() < 0.5` is skipped by this new runtime-filter pruning
guard, but can still be evaluated once in `_evaluate_constant_filters()` and
close the split based on a single random/error-preserving result. Please apply
the same safety gate before constant-filter pruning, leaving unsafe
constant-only predicates for the normal row-level filter path.
##########
be/src/format_v2/table_reader.cpp:
##########
@@ -790,6 +790,15 @@ Status
TableReader::_evaluate_partition_prune_conjuncts(const VExprContextSPtrs&
for (const auto& conjunct : conjuncts) {
DORIS_CHECK(conjunct != nullptr);
DORIS_CHECK(conjunct->root() != nullptr);
+ const auto root = conjunct->root();
+ const auto impl = root->get_impl();
+ const auto predicate = impl != nullptr ? impl : root;
+ // Split pruning evaluates a predicate once before any file rows are
read. Reordering
Review Comment:
This fixes the v2 TableReader pruning path, but the same runtime-filter
partition-pruning feature is still reachable through the legacy `FileScanner`
fallback. `FileScanOperator` falls back to `FileScanner` when
`enable_file_scanner_v2` is false, during loads, or when a range is not
supported by v2. That path builds `_runtime_filter_partition_prune_ctxs()` by
checking only whether the unwrapped expression references partition slots, then
executes those contexts on the synthetic partition block. There is no
`is_safe_to_execute_on_selected_rows()` check there, so the same
non-deterministic/error-preserving partition-only runtime filter can still
prune a whole split once v1 is selected. Please mirror the safety check in the
legacy partition-prune setup, or otherwise keep the behavior consistent across
the v1/v2 scanner selection.
--
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]
