github-actions[bot] commented on code in PR #66357:
URL: https://github.com/apache/doris/pull/66357#discussion_r3698036618
##########
be/src/format_v2/parquet/parquet_statistics.cpp:
##########
@@ -452,11 +452,9 @@ std::optional<format::LocalColumnId>
file_column_id_by_block_position(
return std::nullopt;
}
-bool has_expr_zonemap_filter(const format::FileScanRequest& request,
- const RuntimeState* runtime_state) {
- if (!expr_zonemap::is_expr_zonemap_filter_enabled(runtime_state)) {
- return false;
- }
+bool has_expr_zonemap_filter(const format::FileScanRequest& request, const
RuntimeState*) {
Review Comment:
**[P1] Preserve error order before making metadata pruning unavoidable**
`request.conjuncts` can contain an earlier localized error-preserving
expression (for example `assert_true(y != 0, 'bad')`) followed by a
ZoneMap-safe `x > 100`. With a PLAIN row group where `y=0` and `max(x)=2`, and
with cost-based conjunct reordering disabled, the old
`enable_expr_zonemap_filter=false` path reaches row evaluation and raises
`bad`; after this change the metadata evaluator skips the unsupported first
conjunct, prunes on the later comparison, and returns no rows. The row
scheduler and partition pruner already treat such expressions as ordering
barriers. Please apply the same safe-prefix fence to both footer and page-index
metadata evaluation before ignoring this switch, and add an error-preservation
test.
##########
be/src/exec/scan/file_scanner_v2.cpp:
##########
@@ -632,9 +632,9 @@ Status FileScannerV2::_prepare_table_reader_split(const
TFileRangeDesc& range,
VExprContextSPtrs conjuncts;
RETURN_IF_ERROR(_build_table_conjuncts(&conjuncts));
VExprContextSPtrs partition_prune_conjuncts;
- if (_state->query_options().enable_runtime_filter_partition_prune) {
- RETURN_IF_ERROR(_build_table_conjuncts(&partition_prune_conjuncts));
- }
+ // FileScannerV2 owns its complete pruning pipeline, so safe partition
predicates must not
+ // inherit the legacy scanner's session gate.
+ RETURN_IF_ERROR(_build_table_conjuncts(&partition_prune_conjuncts));
Review Comment:
**[P2] Skip pruning-clone work when this split has no partition values**
The scanner already has the generated `partition_values` map here, but this
now deep-clones and rewrites every conjunct even when that map is empty.
`TableReader::_evaluate_partition_prune_conjuncts()` immediately returns for an
empty map, and no other reader consumes these clones, so a non-partitioned
many-file scan pays a second expression-tree allocation/traversal per file with
no possible pruning benefit. Please keep pruning unconditional for eligible V2
splits while guarding this clone on a non-empty/eligible partition-value set,
and cover the no-partition path in the unit test.
##########
be/src/exec/scan/file_scanner_v2.cpp:
##########
@@ -632,9 +632,9 @@ Status FileScannerV2::_prepare_table_reader_split(const
TFileRangeDesc& range,
VExprContextSPtrs conjuncts;
RETURN_IF_ERROR(_build_table_conjuncts(&conjuncts));
VExprContextSPtrs partition_prune_conjuncts;
- if (_state->query_options().enable_runtime_filter_partition_prune) {
- RETURN_IF_ERROR(_build_table_conjuncts(&partition_prune_conjuncts));
- }
+ // FileScannerV2 owns its complete pruning pipeline, so safe partition
predicates must not
+ // inherit the legacy scanner's session gate.
Review Comment:
**[P2] Align the public switch contract before treating it as legacy**
`enable_runtime_filter_partition_prune` and `enable_expr_zonemap_filter` are
still forwarded session controls; FE/Thrift describe the latter as controlling
scanner/BE-reader ZoneMap pruning, and V1/older-BE paths still honor both. This
makes the meaning of `false` depend on scanner selection or backend version,
while existing Iceberg/Paimon suites that use `false` as the non-pruned
correctness baseline now exercise the same default-V2 pruning path in both
halves. Please either keep honoring these controls, or update the durable
FE/Thrift/user-facing contract and provide an explicit V2 per-feature
test/escape path before removing those differential baselines; the PR release
note and local `legacy` comment do not update the exposed session semantics.
--
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]
