Copilot commented on code in PR #2370:
URL: https://github.com/apache/auron/pull/2370#discussion_r3548640756
##########
spark-extension-shims-spark/src/test/scala/org/apache/auron/AuronQuerySuite.scala:
##########
@@ -1014,4 +1014,17 @@ class AuronQuerySuite extends AuronQueryTest with
BaseAuronSQLSuite with AuronSQ
|FROM t_filter_agg_2289""".stripMargin)
}
}
+
+ test("test OR pushdown with an unconvertible disjunct for orc table") {
+ withTable("orc_or") {
+ 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
Review Comment:
The comment describing the coercion result is incorrect: comparing string
column `b` to numeric literal `900000` would coerce to something like `cast(b
as double) = 900000.0`, not `= 2.0`. This makes the test rationale
confusing/misleading for future readers.
##########
native-engine/datafusion-ext-plans/src/orc_exec.rs:
##########
@@ -1122,6 +1141,115 @@ mod tests {
assert_eq!(condition_count, 3);
}
+ #[test]
+ fn test_or_with_unconvertible_disjunct_not_pushed() {
+ let schema = create_test_schema();
+ // id = 1 OR (id = age)
+ // The second disjunct compares two columns and cannot be converted.
+ // The whole OR must NOT push down a narrowed predicate, otherwise the
+ // reader would skip row groups that only satisfy the dropped disjunct.
+ let id = Arc::new(Column::new("id", 0));
+ let lit1 = Arc::new(Literal::new(ScalarValue::Int32(Some(1))));
+ let conv = Arc::new(BinaryExpr::new(id.clone(), Operator::Eq, lit1));
+
+ let age = Arc::new(Column::new("age", 2));
+ let unconv = Arc::new(BinaryExpr::new(id, Operator::Eq, age));
+
+ let or_expr = Arc::new(BinaryExpr::new(conv, Operator::Or, unconv));
+
+ let result = convert_predicate_to_orc(Some(or_expr), &schema);
+ assert!(
+ result.is_none(),
+ "OR with an unconvertible disjunct must not push down, got:
{result:?}"
+ );
+ }
+
+ #[test]
+ fn test_or_with_unconvertible_and_branch_not_pushed() {
+ let schema = create_test_schema();
+ // (id = age AND age = score) OR (id = 2)
+ // The first disjunct is an AND of two column-column comparisons,
neither
+ // of which converts, so the AND yields no predicate. That disjunct
must
+ // poison the whole OR rather than being silently dropped. This mirrors
+ // the production bug where `cast(type)=2 AND cast(gjo)=1` was dropped.
+ let id = Arc::new(Column::new("id", 0));
+ let age = Arc::new(Column::new("age", 2));
+ let id_eq_age = Arc::new(BinaryExpr::new(id.clone(), Operator::Eq,
age.clone()));
+ let age_eq_score = Arc::new(BinaryExpr::new(
+ age,
+ Operator::Eq,
+ Arc::new(Column::new("score", 3)),
+ ));
+ let and_branch = Arc::new(BinaryExpr::new(id_eq_age, Operator::And,
age_eq_score));
+
+ let lit2 = Arc::new(Literal::new(ScalarValue::Int32(Some(2))));
+ let id_eq_2 = Arc::new(BinaryExpr::new(id, Operator::Eq, lit2));
+
+ let or_expr = Arc::new(BinaryExpr::new(and_branch, Operator::Or,
id_eq_2));
+
+ let result = convert_predicate_to_orc(Some(or_expr), &schema);
+ assert!(
+ result.is_none(),
+ "OR whose disjunct is a fully-unconvertible AND must not push
down, got: {result:?}"
+ );
+ }
+
+ #[test]
+ fn test_and_keeps_convertible_conjunct_when_or_unconvertible() {
+ let schema = create_test_schema();
+ // name = "x" AND (id = 1 OR id = age)
+ // The OR is unconvertible, but it is an AND conjunct. Dropping it only
+ // loosens the pushed predicate, so the convertible name = "x" conjunct
+ // must still push down.
+ let name = Arc::new(Column::new("name", 1));
+ let name_lit =
Arc::new(Literal::new(ScalarValue::Utf8(Some("x".to_string()))));
+ let name_eq = Arc::new(BinaryExpr::new(name, Operator::Eq, name_lit));
+
+ let id = Arc::new(Column::new("id", 0));
+ let lit1 = Arc::new(Literal::new(ScalarValue::Int32(Some(1))));
+ let id_eq_1 = Arc::new(BinaryExpr::new(id.clone(), Operator::Eq,
lit1));
+ let age = Arc::new(Column::new("age", 2));
+ let id_eq_age = Arc::new(BinaryExpr::new(id, Operator::Eq, age));
+ let or_expr = Arc::new(BinaryExpr::new(id_eq_1, Operator::Or,
id_eq_age));
+
+ let and_expr = Arc::new(BinaryExpr::new(name_eq, Operator::And,
or_expr));
+
+ 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}"
+ );
Review Comment:
The assertions in this test are too weak: `debug_str.contains("\"name\"") &&
debug_str.contains("Equal")` would still pass if the unconvertible OR were
incorrectly narrowed to a single disjunct (e.g., `id = 1`) and then combined as
an AND. Asserting the exact pushed predicate string makes this regression
detectable and also implicitly verifies the OR was dropped.
--
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]
