andygrove commented on code in PR #5773:
URL: https://github.com/apache/datafusion-comet/pull/5773#discussion_r3960613505
##########
spark/src/test/scala/org/apache/comet/CometIcebergSystemFunctionSuite.scala:
##########
@@ -335,6 +336,57 @@ class CometIcebergSystemFunctionSuite
Unsupported(Some(CometIcebergTruncate.DecimalNote)))
}
+ test("ApplyFunctionExpression reaches the same handlers as StaticInvoke") {
+ // Spark's `V2ExpressionUtils.resolveScalarFunction` wraps a DSv2 catalog
scalar function as
+ // `ApplyFunctionExpression` when the implementation class does not expose
a static `invoke`
+ // magic method. Iceberg's per-type functions carry the same class as
their identity on both
+ // paths, so a single set of handlers keyed by class name must cover both.
+ val value = AttributeReference("v", IntegerType)()
+ val bucketIntCls =
+
Class.forName("org.apache.iceberg.spark.functions.BucketFunction$BucketInt")
+ val bucketInt =
+
bucketIntCls.getDeclaredConstructor().newInstance().asInstanceOf[ScalarFunction[_]]
+ val expr = ApplyFunctionExpression(bucketInt, Seq(Literal(4), value))
+ assert(CometApplyFunctionExpression.getSupportLevel(expr) == Compatible())
+
+ // The same argument-shape checks that gate the StaticInvoke path have to
gate this one, or a
+ // zero-bucket call would land natively and diverge from Iceberg's own
ArithmeticException.
+ val zeroBuckets = ApplyFunctionExpression(bucketInt, Seq(Literal(0),
value))
+
assert(CometApplyFunctionExpression.getSupportLevel(zeroBuckets).isInstanceOf[Unsupported])
+ }
+
+ test("Iceberg handler map is keyed by Iceberg implementation class names") {
+ // Guards against a rename of one of Iceberg's per-type implementation
classes silently
+ // dropping the native path: if any of these classes is on the classpath,
its name has to be
+ // in the handler map.
+ val expected = Seq(
+ "org.apache.iceberg.spark.functions.BucketFunction$BucketInt" ->
CometIcebergBucket,
+ "org.apache.iceberg.spark.functions.BucketFunction$BucketLong" ->
CometIcebergBucket,
+ "org.apache.iceberg.spark.functions.TruncateFunction$TruncateInt" ->
CometIcebergTruncate,
+ "org.apache.iceberg.spark.functions.TruncateFunction$TruncateString" ->
CometIcebergTruncate)
+ expected.foreach { case (className, expectedHandler) =>
+ assert(
+
CometIcebergSystemFunctions.handlers.get(className).contains(expectedHandler),
+ s"missing handler for $className")
+ }
+ }
+
+ test("Iceberg handler map is keyed by Iceberg implementation class names") {
Review Comment:
Fixed in 85bd18354 -- the second copy is gone.
Thanks for tracing this to suite construction rather than the test body.
That is what made the failure look unrelated to the test's own preconditions:
the `assume(icebergAvailable, ...)` in this suite's `test` override never gets
a chance to run, so the abort happened even on jobs where Iceberg is absent and
every test would otherwise have been skipped.
Both new tests now run for real. Locally: 13 succeeded, 0 failed, 0 canceled
on the default profile (Spark 4.1, Scala 2.13) and on `-Pspark-3.4` (Scala
2.12). The `canceled 0` is the part I checked deliberately, since a skipped
suite would have reported success just as quietly as before.
##########
spark/src/test/scala/org/apache/comet/CometIcebergSystemFunctionSuite.scala:
##########
@@ -335,6 +336,57 @@ class CometIcebergSystemFunctionSuite
Unsupported(Some(CometIcebergTruncate.DecimalNote)))
}
+ test("ApplyFunctionExpression reaches the same handlers as StaticInvoke") {
+ // Spark's `V2ExpressionUtils.resolveScalarFunction` wraps a DSv2 catalog
scalar function as
+ // `ApplyFunctionExpression` when the implementation class does not expose
a static `invoke`
+ // magic method. Iceberg's per-type functions carry the same class as
their identity on both
+ // paths, so a single set of handlers keyed by class name must cover both.
+ val value = AttributeReference("v", IntegerType)()
+ val bucketIntCls =
+
Class.forName("org.apache.iceberg.spark.functions.BucketFunction$BucketInt")
+ val bucketInt =
+
bucketIntCls.getDeclaredConstructor().newInstance().asInstanceOf[ScalarFunction[_]]
Review Comment:
Fixed in 85bd18354 -- now constructed through
`getDeclaredConstructor(classOf[DataType]).newInstance(IntegerType)`.
I confirmed the signature independently with `javap` against the four
runtimes these profiles actually build against, and `BucketInt(DataType)` is
the only declared constructor in every one:
| runtime | constructor |
|---|---|
| iceberg-spark-runtime-3.4_2.12:1.5.2 | `BucketInt(DataType)` |
| iceberg-spark-runtime-3.5_2.13:1.8.1 | `BucketInt(DataType)` |
| iceberg-spark-runtime-4.0_2.13:1.10.0 | `BucketInt(DataType)` |
| iceberg-spark-runtime-4.1_2.13:1.11.0 | `BucketInt(DataType)` |
So this would indeed have failed the moment the duplicate registration
stopped masking it, including on the 3.4 profile that was outside the set you
checked.
--
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]
