sunchao commented on code in PR #5359:
URL: https://github.com/apache/datafusion-comet/pull/5359#discussion_r3788166476
##########
native/spark-expr/src/comet_scalar_funcs.rs:
##########
@@ -260,6 +260,13 @@ pub fn create_comet_physical_fun_with_eval_mode(
let func = Arc::new(crate::string_funcs::spark_levenshtein);
make_comet_scalar_udf!("levenshtein", func, without data_type)
}
+ // Registry UDFs (including datafusion-spark) cannot receive
fail_on_error.
+ _ if fail_on_error => Err(DataFusionError::Execution(format!(
Review Comment:
[P1] Preserve existing `make_time` registry dispatch before rejecting
`fail_on_error=true`
The Spark 4.1+ shim already calls
`scalarFunctionExprToProtoWithReturnType("make_time", s.dataType, true, ...)`,
but `SparkMakeTime` is registered only in `all_scalar_functions()` and has no
dedicated match arm. This branch therefore rejects every nonconstant
`make_time` query during native planning, including valid inputs.
`SparkMakeTime` already implements Spark's always-throw semantics correctly, so
please add an explicit `"make_time"` match arm or a narrowly safe exemption,
and cover `create_comet_physical_fun("make_time", ..., Some(true))` in the
regression test.
##########
spark/src/test/scala/org/apache/comet/serde/CometScalarFunctionSuite.scala:
##########
@@ -0,0 +1,166 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.comet.serde
+
+import org.apache.spark.sql.CometTestBase
+import org.apache.spark.sql.catalyst.expressions.{Abs, Cos, Expression,
Literal, Unevaluable}
+import org.apache.spark.sql.types.{DataType, IntegerType}
+
+import org.apache.comet.{CometExplainInfo, CometSparkSessionExtensions}
+
+/**
+ * Synthetic expression whose constructor declares `evalMode`, used to prove
class-level detection
+ * without depending on Spark-version-specific arithmetic field names.
+ */
+case class TestEvalModeExpression(child: Expression, evalMode: Boolean)
+ extends Expression
+ with Unevaluable {
+ override def children: Seq[Expression] = Seq(child)
+ override def nullable: Boolean = child.nullable
+ override def dataType: DataType = IntegerType
+ override protected def withNewChildrenInternal(
+ newChildren: IndexedSeq[Expression]): Expression =
+ copy(child = newChildren.head)
+}
+
+/**
+ * Synthetic expression whose constructor declares `nullOnOverflow`, matching
markers such as
+ * Spark's `MakeDecimal`.
+ */
+case class TestNullOnOverflowExpression(child: Expression, nullOnOverflow:
Boolean)
+ extends Expression
+ with Unevaluable {
+ override def children: Seq[Expression] = Seq(child)
+ override def nullable: Boolean = child.nullable
+ override def dataType: DataType = IntegerType
+ override protected def withNewChildrenInternal(
+ newChildren: IndexedSeq[Expression]): Expression =
+ copy(child = newChildren.head)
+}
+
+class CometScalarFunctionSuite extends CometTestBase {
Review Comment:
[P1] Register the new suite in both CI workflow matrices
`CometScalarFunctionSuite` is not listed in either
`.github/workflows/pr_build_linux.yml` or
`.github/workflows/pr_build_macos.yml`. The mandatory preflight runs `python3
dev/ci/check-suites.py`, which requires every `*Suite.scala` in both workflows;
on this head it exits 255 with `Suite not found in workflow
.github/workflows/pr_build_linux.yml:
org.apache.comet.serde.CometScalarFunctionSuite`. Please add the suite to the
`expressions` bucket in both workflows; otherwise all downstream CI jobs are
blocked.
##########
spark/src/main/scala/org/apache/comet/serde/CometScalarFunction.scala:
##########
@@ -21,14 +21,53 @@ package org.apache.comet.serde
import org.apache.spark.sql.catalyst.expressions.{Attribute, Expression}
+import org.apache.comet.CometSparkSessionExtensions.withFallbackReason
import org.apache.comet.serde.ExprOuterClass.Expr
import org.apache.comet.serde.QueryPlanSerde.{exprToProtoInternal,
scalarFunctionExprToProto}
/** Serde for scalar function. */
case class CometScalarFunction[T <: Expression](name: String) extends
CometExpressionSerde[T] {
override def convert(expr: T, inputs: Seq[Attribute], binding: Boolean):
Option[Expr] = {
+ if (CometScalarFunction.isAnsiSensitive(expr)) {
+ withFallbackReason(
+ expr,
+ s"${expr.nodeName} carries failOnError/evalMode/nullOnOverflow and
cannot use " +
+ s"CometScalarFunction('$name'). Prefer name-based ANSI/try variants
" +
+ "(e.g. parse_url / try_parse_url), or a custom serde with " +
+ "scalarFunctionExprToProtoWithReturnType plus a native match arm
that " +
+ "consumes fail_on_error.")
+ return None
+ }
val childExpr = expr.children.map(exprToProtoInternal(_, inputs, binding))
val optExpr = scalarFunctionExprToProto(name, childExpr: _*)
optExpr
}
}
+
+object CometScalarFunction {
+
+ /** Product field names that indicate ANSI / eval-mode sensitive Spark
expressions. */
+ private val AnsiSensitiveFields: Set[String] =
+ Set("failOnError", "evalMode", "nullOnOverflow")
+
+ /**
+ * True when the Spark expression case class declares an ANSI-related
constructor field. Used to
+ * reject miswiring via plain [[CometScalarFunction]].
+ */
+ private[serde] def isAnsiSensitive(expr: Expression): Boolean = {
+ expr match {
+ case p: Product =>
+ p.productElementNames.exists(AnsiSensitiveFields.contains)
Review Comment:
[P1] Keep ANSI-field detection compatible with Scala 2.12
`scala.Product.productElementNames` exists in Scala 2.13 but not Scala 2.12.
The supported Spark 3.4 and 3.5 profiles include Scala 2.12, so their
lint/build matrix entries cannot compile this source: `value
productElementNames is not a member of Product`. I reproduced the failure with
Scala 2.12.18 and confirmed the same expression compiles with Scala 2.13.16.
Please use a Scala-2.12-compatible way to inspect these fields, or introduce
version-specific shims.
--
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]
