sunchao commented on code in PR #5773:
URL: https://github.com/apache/datafusion-comet/pull/5773#discussion_r3959825654
##########
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:
### Correctness
[P2] Supply the required BucketInt constructor argument
Could you construct `BucketInt` through its `DataType` constructor, passing
`IntegerType`, or obtain it through Iceberg binding? `BucketInt` has no
no-argument constructor in the Iceberg 1.8.1, 1.10.0 and 1.11.0 runtimes used
by these test profiles. I verified the signatures and reproduced
`NoSuchMethodException: BucketFunction$BucketInt.<init>()` from this exact
lookup with an isolated reflection check against all three jars. Once the
duplicate registration is fixed, this test will fail here before reaching
either support assertion.
##########
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:
### Correctness
[P2] Remove the duplicate test registration
Could you remove this second copy of the test already registered at line
358? ScalaTest rejects the duplicate name while constructing the suite, before
any test body or the `icebergAvailable` assumption runs. The current Spark 4.0
and 4.2 scan jobs both abort `CometIcebergSystemFunctionSuite` with `Duplicate
test name: Iceberg handler map is keyed by Iceberg implementation class names`,
so none of this suite validates the change. Both jobs ran the assigned
base/head merge. Removing the duplicated block and rerunning the suite
addresses this failure.
--
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]
