Flyangz commented on code in PR #2370:
URL: https://github.com/apache/auron/pull/2370#discussion_r3526776010
##########
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:
lyne7-sc raised a similar point above, and I have replied there. To
summarize, while the NOT scenario is possible, I don't think we need to include
it here because:
1. It's almost impossible to trigger in production. Spark's default
`BooleanSimplification` optimizer rule will rewrite `NOT(id = 1 AND id = age)`
to `id != 1 OR id != age`. The bug only surfaces if this rule is turned off.
Here is a test case to verify this:
```scala
test("test NOT pushdown over AND with an unconvertible operand for orc
table") {
withTable("orc_not") {
sql("create table orc_not(id int, b string) using orc")
sql("insert into orc_not select cast(id as int), cast(id as string)
from range(0, 1000000)")
withSQLConf(
"spark.sql.optimizer.excludedRules" ->
"org.apache.spark.sql.catalyst.optimizer.BooleanSimplification") {
checkSparkAnswerAndOperator(
"select count(*) from orc_not where not(id < 900000 and b = 500)")
}
}
}
```
2. To keep this PR focused and clean, I believe fixing the NOT case and
adding the test would be better suited for a separate follow-up issue.
--
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]
