weiqingy commented on code in PR #2370:
URL: https://github.com/apache/auron/pull/2370#discussion_r3522179853
##########
native-engine/datafusion-ext-plans/src/orc_exec.rs:
##########
@@ -473,33 +473,44 @@ fn collect_and_predicates(
/// Recursively collect all OR sub-conditions and flatten nested OR
/// structures.
+///
+/// Returns `false` if any disjunct cannot be converted. OR pushdown must be
+/// all-or-nothing: a pushed predicate is only used to skip row groups whose
+/// statistics prove no row can match, so it must be implied by the true filter
+/// (`true_filter => pushed`). Dropping a disjunct narrows the OR into a subset
+/// of the true filter, which makes the reader skip row groups that actually
+/// contain matching rows. (Dropping AND conjuncts only loosens the predicate,
+/// so that stays safe.)
fn collect_or_predicates(
expr: &Arc<dyn datafusion::physical_expr::PhysicalExpr>,
schema: &SchemaRef,
predicates: &mut Vec<Predicate>,
-) {
+) -> bool {
// Handle short-circuit OR expression (SCOrExpr)
if let Some(sc_or) = expr.as_any().downcast_ref::<SCOrExpr>() {
// Recursively collect OR sub-conditions from both sides
- collect_or_predicates(&sc_or.left, schema, predicates);
- collect_or_predicates(&sc_or.right, schema, predicates);
- return;
+ return collect_or_predicates(&sc_or.left, schema, predicates)
+ && collect_or_predicates(&sc_or.right, schema, predicates);
}
// Handle BinaryExpr with OR operator
if let Some(binary) = expr.as_any().downcast_ref::<BinaryExpr>() {
if matches!(binary.op(), Operator::Or) {
// Recursively collect OR sub-conditions from both sides
- collect_or_predicates(binary.left(), schema, predicates);
- collect_or_predicates(binary.right(), schema, predicates);
- return;
+ return collect_or_predicates(binary.left(), schema, predicates)
+ && collect_or_predicates(binary.right(), schema, predicates);
}
}
- // Not an OR expression, convert the whole expression
- // (could be AND, comparison, IS NULL, etc.)
- if let Some(pred) = convert_expr_to_orc(expr, schema) {
- predicates.push(pred);
+ // Not an OR expression, convert the whole expression as a single disjunct
+ // (could be AND, comparison, IS NULL, etc.). If it cannot be converted,
the
+ // entire OR is unpushable.
+ match convert_expr_to_orc(expr, schema) {
Review Comment:
Could this still have a similar implication issue through `NOT`? `NotExpr`
converts its child with `convert_expr_to_orc` and then wraps it in
`Predicate::not(...)`, but child conversion can be partial for `AND` by
dropping unconvertible conjuncts. That is safe before negation, but maybe not
after it.
For example, `NOT(id = 1 AND id = age)` could become `NOT(id = 1)` if `id =
age` is not convertible. That seems narrower than the original predicate and
might let ORC pruning skip row groups with valid rows.
Would it be worth making `NOT` conversion all-or-nothing, or adding a
regression test 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]
