github-actions[bot] commented on code in PR #67774:
URL: https://github.com/apache/doris/pull/67774#discussion_r4107483514
##########
be/src/exprs/expr_zonemap_filter.cpp:
##########
@@ -250,6 +250,39 @@ std::optional<SlotLiteral> extract_slot_and_literal(const
VExprSPtrs& args) {
return std::nullopt;
}
+std::optional<SlotSlot> extract_slot_and_slot(const VExprSPtrs& args) {
+ if (args.size() != 2) {
+ return std::nullopt;
+ }
+ auto left = std::dynamic_pointer_cast<VSlotRef>(args[0]);
+ auto right = std::dynamic_pointer_cast<VSlotRef>(args[1]);
+ if (left == nullptr || right == nullptr) {
+ return std::nullopt;
+ }
+ return SlotSlot {.left_slot_index = left->column_id(),
+ .left_type = left->data_type(),
+ .right_slot_index = right->column_id(),
+ .right_type = right->data_type()};
+}
+
+bool contains_slot_slot_comparison(const VExprSPtr& expr) {
+ if (expr == nullptr) {
+ return false;
+ }
+ // A comparison node's two operands are its children, so a slot-vs-slot
leaf is a node whose
+ // children are two VSlotRef. extract_slot_and_slot accepts a same-column
pair too, which is the
+ // point: `a < a` has one column id but is still slot-vs-slot.
+ if (extract_slot_and_slot(expr->children()).has_value()) {
Review Comment:
[P2] Limit this detector to actual comparison nodes
`extract_slot_and_slot(expr->children())` also succeeds for any
non-comparison binary node with two slot children, such as `add(a, a)`. That
makes both v1 gates reject otherwise useful compounds. For example, `((a < 0
AND a + a > 10) OR a > 100)` is zone-map-capable: `VCompoundPred` skips the
unsupported second AND child, and for `a=[10,20]` the old path proved both OR
branches no-match and pruned the unit. The current recursion reaches `add(a,a)`
and drops the whole conjunct. Please require this node itself to be one of the
comparison functions before treating its children as a slot-slot comparison,
and cover the Row Group and Page Index paths with a non-comparison two-slot
child.
##########
be/src/format/parquet/vparquet_reader.cpp:
##########
@@ -1648,11 +1658,24 @@ Status
ParquetReader::_process_expr_zonemap_filter(const tparquet::RowGroup& row
return Status::OK();
}
+ // The v1 Parquet reader is being removed, so it does not take on the new
slot-vs-slot pruning.
+ // Restrict this path to single-column conjuncts that carry no
slot-vs-slot leaf: counting
+ // distinct columns alone is not enough, because a same-column pair like
`a < a` still has one
+ // column id. A two-slot shape falls back to no pruning here. Native and
v2 keep it.
std::set<int> column_ids;
+ VExprContextSPtrs single_slot_conjuncts;
for (const auto& conjunct : all_conjuncts) {
- if (conjunct->root() != nullptr &&
conjunct->root()->can_evaluate_zonemap_filter()) {
- conjunct->root()->collect_slot_column_ids(column_ids);
+ if (conjunct->root() == nullptr ||
!conjunct->root()->can_evaluate_zonemap_filter()) {
+ continue;
}
+ std::set<int> conjunct_column_ids;
+ conjunct->root()->collect_slot_column_ids(conjunct_column_ids);
+ if (conjunct_column_ids.size() != 1 ||
Review Comment:
[P2] Preserve multi-column slot-literal compound pruning
This distinct-column check also drops established v1 row-group pruning for
compounds that contain no slot-vs-slot leaf. With
`enable_file_scanner_v2=false`, `a < 0 OR b < 0` has two IDs and is omitted
here; for a row group with `a=[10,20]` and `b=[30,40]`, the old path evaluated
both children and pruned the group, while the current path always reads it.
`contains_slot_slot_comparison()` already supplies the required safety fence.
Please retain every conjunct for which that check is false and insert all of
its collected IDs into `column_ids`, with a v1 multi-column slot-literal AND/OR
regression test.
##########
be/src/format/parquet/vparquet_reader.cpp:
##########
@@ -1648,11 +1658,24 @@ Status
ParquetReader::_process_expr_zonemap_filter(const tparquet::RowGroup& row
return Status::OK();
}
+ // The v1 Parquet reader is being removed, so it does not take on the new
slot-vs-slot pruning.
+ // Restrict this path to single-column conjuncts that carry no
slot-vs-slot leaf: counting
+ // distinct columns alone is not enough, because a same-column pair like
`a < a` still has one
+ // column id. A two-slot shape falls back to no pruning here. Native and
v2 keep it.
std::set<int> column_ids;
+ VExprContextSPtrs single_slot_conjuncts;
for (const auto& conjunct : all_conjuncts) {
- if (conjunct->root() != nullptr &&
conjunct->root()->can_evaluate_zonemap_filter()) {
- conjunct->root()->collect_slot_column_ids(column_ids);
+ if (conjunct->root() == nullptr ||
!conjunct->root()->can_evaluate_zonemap_filter()) {
+ continue;
}
+ std::set<int> conjunct_column_ids;
+ conjunct->root()->collect_slot_column_ids(conjunct_column_ids);
+ if (conjunct_column_ids.size() != 1 ||
+ expr_zonemap::contains_slot_slot_comparison(conjunct->root())) {
Review Comment:
[P2] Preserve safe siblings when fencing slot-slot leaves
Rejecting the whole conjunct also removes old v1 pruning that does not
depend on the unsafe leaf. Before this PR, `((a < 0 AND c < d) OR a > 100)` was
zone-map-capable because the AND skipped unsupported `c < d`; for a row group
with `a=[10,20]`, both OR branches were `kNoMatch`, so the group was pruned
regardless of c/d. This check now drops the entire tree. This is separate from
the nested-leaf safety requirement: please keep the actual slot-slot leaf
unavailable to v1 while retaining proofs from safe siblings, and add a
row-group regression for this shape.
--
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]
