comphead commented on code in PR #4653:
URL: https://github.com/apache/datafusion-comet/pull/4653#discussion_r4277235378
##########
dev/diffs/3.5.9.diff:
##########
@@ -1076,24 +1076,25 @@ index 3cf2bfd17ab..5bcf9478e9b 100644
val hiveVersion = "2.3.9"
// transitive=false, only download specified jar
diff --git
a/sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestSuite.scala
b/sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestSuite.scala
-index 71af1fd69c3..81a04c93c9c 100644
+index 71af1fd69c3..46340ba5740 100644
--- a/sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestSuite.scala
+++ b/sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestSuite.scala
-@@ -872,9 +872,24 @@ class SQLQueryTestSuite extends QueryTest with
SharedSparkSession with SQLHelper
+@@ -872,9 +872,25 @@ class SQLQueryTestSuite extends QueryTest with
SharedSparkSession with SQLHelper
s"Schema did not match for query #$i\n${expected.sql}: $output") {
output.schema
}
- assertResult(expected.output, s"Result did not match" +
-+ // Comet may surface errors as `CometNativeException` instead of the
matching Spark
-+ // exception class when a `ScalaUDF` dispatched into the native plan
evaluates a
-+ // divide-by-zero (DataFusion wraps the typed error so the JNI bridge
cannot downcast it).
-+ // Same category, different surface. Collapse both sides to a
placeholder when this happens
-+ // so the literal compare passes.
-+ // Tracking issue:
https://github.com/apache/datafusion-comet/issues/4517
++ // With parquet filter pushdown, DataFusion 54/55 formats an error
raised while
++ // evaluating a pushed-down predicate into a string, so a
divide-by-zero from a
++ // `ScalaUDF` dispatched into the native plan surfaces as
`CometNativeException`
++ // instead of `SparkArithmeticException`. Collapse both sides to a
placeholder in that
++ // case only. Remove once Comet uses a DataFusion release that includes
Review Comment:
The description says `Closes #4517`, but the query that issue reports
(`udf/postgreSQL/udf-select_having.sql`, `WHERE 1/udf(a) = 1`) is the filter
pushdown path this workaround still covers. I expect that path to still fail at
DataFusion 55.2.0, since `row_filter.rs` formats the error into a string, and
the fix in apache/datafusion#24638 is not in 55.2.0. Merging as written closes
#4517 and also deletes the `Tracking issue: .../issues/4517` comment that
`main` carries in all five diffs, so nothing would track removing the
workaround. Could the description say `Part of #4517`, so GitHub does not close
it, and could the link stay next to the DataFusion one? The description also
still refers to `arithmetic.rs` and `extract_spark_error`, which no longer
exist at this head.
##########
spark/src/test/scala/org/apache/comet/CometCodegenSuite.scala:
##########
@@ -2344,6 +2344,57 @@ class CometCodegenSuite
}
}
+ test("divide-by-zero through dispatched ScalaUDF surfaces
SparkArithmeticException (#4517)") {
+ // When a ScalaUDF is dispatched into the native plan and the outer
expression divides
+ // by the UDF result, the error must surface as
SparkArithmeticException(DIVIDE_BY_ZERO),
+ // not as CometNativeException. Verifies the JNI bridge correctly unwraps
the
+ // DataFusion error chain even when DataFusion 53+ wraps it in
Context/External layers.
+ assertDispatchedUdfArithmeticError("SELECT 1 / identity_int(a) FROM t",
"DIVIDE_BY_ZERO")
+ }
+
+ test("integer overflow through dispatched ScalaUDF surfaces
SparkArithmeticException (#4517)") {
+ // Same path as the divide-by-zero case, but a different SparkError
variant, so a
+ // misclassification in the JNI bridge cannot pass both tests.
+ assertDispatchedUdfArithmeticError(
+ "SELECT identity_int(a) + 2147483647 FROM t",
+ "ARITHMETIC_OVERFLOW")
+ }
+
+ /**
+ * Runs `query` over `t (a INT)` holding 0 and 1 with ANSI enabled, where
the query applies
+ * native arithmetic to the result of the dispatched ScalaUDF
`identity_int`, and asserts the
+ * failure surfaces as `SparkArithmeticException` carrying `errorClass`.
+ */
+ private def assertDispatchedUdfArithmeticError(query: String, errorClass:
String): Unit = {
Review Comment:
This helper re-implements two existing ones. `assertCodegenRan` in
`CometCodegenAssertions` covers the dispatcher activity check, and
`checkSparkError` in `CometTestBase` runs the query on Spark too, rejects a
`CometNativeException` in the cause chain and compares the error class. This
helper only does substring checks on class names and the message. Could it
become `assertCodegenRan { checkSparkError(sql(query), errorClass) }` inside
the ANSI and table setup? I also could not find a `Context` wrapper on the
projection path in DataFusion 55.2.0, so do these two tests fail on `main`? If
they pass there, they overlap `framework/expect_error_class.sql` and the real
regression coverage is the Rust tests. I did not run them.
##########
native/jni-bridge/src/errors.rs:
##########
@@ -2028,6 +2036,59 @@ mod tests {
);
}
+ #[test]
+ fn extract_spark_payload_through_shared() {
Review Comment:
The `Shared` case is already covered twice, by the `shared` assertion this
PR adds to `parquet_external_spark_error_keeps_its_type` and by the `"Shared"`
entry in the error chain test in `error.rs`. Could this test be dropped? The
test at line 1808 is also still named after `parquet_external_spark_error`,
which this PR removes, so a name such as
`extract_spark_payload_through_parquet_external` would fit.
--
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]
