andygrove commented on code in PR #6599:
URL: https://github.com/apache/datafusion-comet/pull/6599#discussion_r4178607513
##########
spark/src/main/scala/org/apache/comet/serde/arrays.scala:
##########
@@ -113,37 +113,24 @@ object CometArrayAppend extends
CometExpressionSerde[ArrayAppend] with ArraysBas
}
}
-object CometArrayContains
- extends CometExpressionSerde[ArrayContains]
- with CodegenDispatchFallback {
-
- private val floatingPointReason: String =
- "Spark compares array elements with ordering.equiv, so -0.0 matches +0.0
and all NaNs match " +
- "each other; Comet's native array_contains compares the raw Arrow values
bitwise"
-
- override def getIncompatibleReasons(): Seq[String] = Seq(floatingPointReason)
-
- override def getSupportLevel(expr: ArrayContains): SupportLevel =
expr.left.dataType match {
- // Native array_contains compares floating-point elements bitwise,
disagreeing with Spark for
- // -0.0/+0.0 and NaN. Report Incompatible (not Unsupported) for
float/double element types (at
- // any nesting level) so the expression routes through the JVM codegen
dispatcher (Spark's own
- // doGenCode) and stays native + Spark-exact under the default config,
while non-float arrays
- // keep the fast native kernel. Under allowIncompatible=true the native
kernel is used
- // as before.
- case ArrayType(elementType, _)
- if SupportLevel.containsType(elementType, classOf[FloatType],
classOf[DoubleType]) =>
- Incompatible(Some(floatingPointReason))
- case _ => Compatible()
- }
+object CometArrayContains extends CometExpressionSerde[ArrayContains] {
override def convert(
expr: ArrayContains,
inputs: Seq[Attribute],
binding: Boolean): Option[ExprOuterClass.Expr] = {
val arrayExprProto = exprToProtoInternal(expr.children.head, inputs,
binding)
val keyExprProto = exprToProtoInternal(expr.children(1), inputs, binding)
-
- scalarFunctionExprToProto("array_contains", arrayExprProto, keyExprProto)
+ // datafusion-spark's array_contains compares floats by their bits.
spark_array_contains
+ // compares them as Spark does, with -0.0 equal to 0.0 and all NaNs equal,
at any depth.
+ val elementType = expr.left.dataType.asInstanceOf[ArrayType].elementType
+ val function =
+ if (SupportLevel.containsType(elementType, classOf[FloatType],
classOf[DoubleType])) {
Review Comment:
I reproduced this one end to end at 46faab056b on Spark 4.1.3. It doesn't
need a collated column, a cast is enough:
```sql
SELECT array_contains(
array(named_struct('d', d, 's', CAST(s AS STRING COLLATE UTF8_LCASE))),
named_struct('d', d, 's', CAST(upper(s) AS STRING COLLATE UTF8_LCASE)))
FROM t
```
The plan is `CometProject` over `CometNativeScan`, and it returns `false`
for `s = 'a'` where Spark returns `true`. With `main`'s serde the same query
goes through the dispatcher and matches Spark, so this is new with this PR.
##########
spark/src/main/scala/org/apache/comet/serde/arrays.scala:
##########
@@ -113,37 +113,24 @@ object CometArrayAppend extends
CometExpressionSerde[ArrayAppend] with ArraysBas
}
}
-object CometArrayContains
- extends CometExpressionSerde[ArrayContains]
- with CodegenDispatchFallback {
-
- private val floatingPointReason: String =
- "Spark compares array elements with ordering.equiv, so -0.0 matches +0.0
and all NaNs match " +
- "each other; Comet's native array_contains compares the raw Arrow values
bitwise"
-
- override def getIncompatibleReasons(): Seq[String] = Seq(floatingPointReason)
-
- override def getSupportLevel(expr: ArrayContains): SupportLevel =
expr.left.dataType match {
- // Native array_contains compares floating-point elements bitwise,
disagreeing with Spark for
- // -0.0/+0.0 and NaN. Report Incompatible (not Unsupported) for
float/double element types (at
- // any nesting level) so the expression routes through the JVM codegen
dispatcher (Spark's own
- // doGenCode) and stays native + Spark-exact under the default config,
while non-float arrays
- // keep the fast native kernel. Under allowIncompatible=true the native
kernel is used
- // as before.
- case ArrayType(elementType, _)
- if SupportLevel.containsType(elementType, classOf[FloatType],
classOf[DoubleType]) =>
- Incompatible(Some(floatingPointReason))
- case _ => Compatible()
- }
+object CometArrayContains extends CometExpressionSerde[ArrayContains] {
override def convert(
expr: ArrayContains,
inputs: Seq[Attribute],
binding: Boolean): Option[ExprOuterClass.Expr] = {
val arrayExprProto = exprToProtoInternal(expr.children.head, inputs,
binding)
val keyExprProto = exprToProtoInternal(expr.children(1), inputs, binding)
-
- scalarFunctionExprToProto("array_contains", arrayExprProto, keyExprProto)
+ // datafusion-spark's array_contains compares floats by their bits.
spark_array_contains
+ // compares them as Spark does, with -0.0 equal to 0.0 and all NaNs equal,
at any depth.
+ val elementType = expr.left.dataType.asInstanceOf[ArrayType].elementType
+ val function =
+ if (SupportLevel.containsType(elementType, classOf[FloatType],
classOf[DoubleType])) {
+ "spark_array_contains"
+ } else {
+ "array_contains"
+ }
+ scalarFunctionExprToProto(function, arrayExprProto, keyExprProto)
Review Comment:
Confirmed this end to end too. With `spark.sql.ansi.enabled=true`,
`array_contains(a, CAST(s AS DOUBLE))` over the rows `(NULL, 'bad')` and
`(array(1.0D), '1')` fails here with `CAST_INVALID_INPUT`. On `main` it goes
through the dispatcher and returns `NULL, true` like Spark. The same query over
an `ARRAY<INT>` already fails on `main`, so that part is older and fits #6006,
but for float arrays this PR is what removes the protection.
`CometArrayJoin.orderInsensitive` in the same file handles this by keeping
anything other than a literal or a column read on the dispatcher. Could
`CometArrayContains` do the same for the value argument when the array can be
null?
--
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]
