[AURON #2369] fix incorrect ORC predicate pushdown with OR#2370
[AURON #2369] fix incorrect ORC predicate pushdown with OR#2370Flyangz wants to merge 2 commits into
Conversation
lyne7-sc
left a comment
lyne7-sc
left a comment
There was a problem hiding this comment.
Nice fix! One small test coverage thought: maybe consider adding a similar regression test with SCOrExpr(id = 1, id = age). The implementation updates both BinaryExpr::Or and SCOrExpr, while the new mixed convertible/unconvertible OR tests only cover the BinaryExpr::new(..., Operator::Or, ...) path.
While reviewing this, I also noticed a related existing case around NOT: NOT(id = 5 AND unsupported_expr) seems like it could become NOT(id = 5) if the inner AND is partiallyconverted. That would be narrower than the original filter and could prune matching rows. This seems to be an existing issue, but it may be worth a follow-up issue/PR.
Flyangz
force-pushed
the
bugfix/open-orc-or-pushdown
branch
from
d29a43c to
fc4377a
Compare
Thanks for the review! I have added the corresponding unit test for SCOrExpr as you suggested. Regarding your second point about the NOT issue, I agree that it should be tracked and fixed in a follow-up issue/PR. That being said, in a typical production environment, the |
| // 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) { |
There was a problem hiding this 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?
There was a problem hiding this 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:
- It's almost impossible to trigger in production. Spark's default
BooleanSimplificationoptimizer rule will rewriteNOT(id = 1 AND id = age)toid != 1 OR id != age. The bug only surfaces if this rule is turned off. Here is a test case to verify this:
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)")
}
}
}- 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.
There was a problem hiding this comment.
Pull request overview
This PR fixes a correctness bug in ORC predicate pushdown where OR filters could be incorrectly narrowed when some disjuncts are not convertible to ORC predicates, leading to row-group pruning that drops matching rows. It makes OR pushdown all-or-nothing (while still allowing safe partial pushdown for surrounding AND conjuncts) and adds unit tests to prevent regressions.
Changes:
- Make
ORpredicate pushdown all-or-nothing by havingcollect_or_predicatesfail the wholeORconversion if any disjunct is unconvertible. - Add Rust unit tests covering unconvertible disjuncts/branches in
OR, includingSCOrExpr, plusANDbehavior when theORside is unpushable. - Add a Spark integration-style test reproducing the reported “mixed-convertibility OR” scenario for ORC tables.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| spark-extension-shims-spark/src/test/scala/org/apache/auron/AuronQuerySuite.scala | Adds an ORC query test reproducing the incorrect OR pushdown behavior in Spark/Auron integration. |
| native-engine/datafusion-ext-plans/src/orc_exec.rs | Changes OR pushdown to be all-or-nothing and adds focused unit tests for the new semantics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| sql("create table orc_or(id int, b string) using orc") | ||
| // enough rows so `id` statistics differ across row groups and pruning kicks in | ||
| sql("insert into orc_or select cast(id as int), cast(id as string) from range(0, 1000000)") | ||
| // `b = 900000` (string col vs int literal) -> cast(b as double)=2.0 -> not convertible |
| let result = convert_predicate_to_orc(Some(and_expr), &schema); | ||
| assert!(result.is_some()); | ||
| let debug_str = format!("{:?}", result.expect("Expected valid ORC predicate")); | ||
| // Only the name = "x" conjunct survives; the unconvertible OR is dropped. | ||
| assert!( | ||
| debug_str.contains("\"name\"") && debug_str.contains("Equal"), | ||
| "Expected name = \"x\" to push down, got: {debug_str}" | ||
| ); | ||
| assert!( | ||
| !debug_str.contains("Or("), | ||
| "Unconvertible OR must not appear in the pushed predicate, got: {debug_str}" | ||
| ); |
Which issue does this PR close?
Closes #2369
Rationale for this change
Fix some ORC predicate pushdown with OR losing data.
What changes are included in this PR?
around the OR still push down safely.
Are there any user-facing changes?
no
How was this patch tested?
unit tests