sam-1112 commented on code in PR #5409:
URL: https://github.com/apache/datafusion-comet/pull/5409#discussion_r3834768115
##########
spark/src/main/scala/org/apache/comet/serde/strings.scala:
##########
@@ -178,29 +179,56 @@ object CometStringReplace
extends CometScalarFunction[StringReplace]("replace")
with NativeOptInAvailable {
+ /**
+ * Native DataFusion `replace` differs from Spark only when the search
string is empty (Spark
+ * returns `src` unchanged; DataFusion inserts the replacement between every
character). That
+ * case is decidable at plan time when `search` is a literal.
+ *
+ * The native kernel is also byte-level `UTF8_BINARY` only, so non-default
collations stay on
+ * the dispatcher. https://github.com/apache/datafusion-comet/issues/4496
+ */
+ private def nativeSafeSearchSubset(expr: StringReplace): Boolean = {
+ val children = expr.children
+ if (children.length != 3) {
+ return false
+ }
+ val searchIsNonEmptyLiteral = children(1) match {
+ case Literal(v: UTF8String, _) => v != null && v.numBytes() > 0
+ case _ => false
+ }
+ val utf8BinaryCollation =
+ !children.exists(c => QueryPlanSerde.isStringCollationType(c.dataType))
+ utf8BinaryCollation && searchIsNonEmptyLiteral
+ }
+
+ override def getCompatibleNotes(): Seq[String] =
+ Seq(
+ "When `search` is a non-empty `UTF8_BINARY` literal, Comet evaluates
`replace` natively " +
+ "by default.")
+
override def getIncompatibleReasons(): Seq[String] =
Seq("Produces different results from Spark when the search string is
empty")
override def getSupportLevel(expr: StringReplace): SupportLevel =
- if (!CometConf.isExprAllowIncompat(getExprConfigName(expr))) {
+ if (CometConf.isExprAllowIncompat(getExprConfigName(expr)) ||
nativeSafeSearchSubset(expr)) {
+ Compatible()
+ } else {
Compatible(nativeOptIn =
Some(NativeOptIn(CometConf.getExprAllowIncompatConfigKey(getExprConfigName(expr)))))
- } else {
- Compatible()
}
override def convert(
expr: StringReplace,
inputs: Seq[Attribute],
binding: Boolean): Option[Expr] = {
- if (CometConf.isExprAllowIncompat(getExprConfigName(expr))) {
- // The native DataFusion `replace` avoids the JVM allocations of the
codegen
- // dispatcher but is not Spark-compatible for an empty search string, so
it is
- // only used when incompatibility is explicitly allowed.
+ if (CometConf.isExprAllowIncompat(getExprConfigName(expr)) ||
nativeSafeSearchSubset(expr)) {
+ // Native DataFusion `replace` matches Spark when search is a non-empty
UTF8_BINARY
+ // literal (the common case, selected by default) and when the user has
opted in.
super.convert(expr, inputs, binding)
Review Comment:
Agreed. Spark's ternary `eval` / `doGenCode` skips the replacement when
`src` is NULL, so this query returns `NULL` and `'1.0'` under ANSI. The native
path evaluates every child for the batch first, so `1 / 0` still runs and
raises `DIVIDE_BY_ZERO`. I am not trying to prove in general whether an
arbitrary replacement can throw. The default native-safe subset now only
accepts a replacement that is a short well-formed literal, a null literal, or a
column (`Attribute` / `BoundReference`). `CAST(1 / n AS STRING)` is none of
those, so it stays on the dispatcher. `CometCodegenSuite` covers the
reproducer: Parquet rows `(NULL, 0)` and `('a', 1)`, ANSI on, `replace(s, 'a',
CAST(1 / n AS STRING))`. The result matches Spark and EXPLAIN still shows `JVM
codegen dispatcher: replace`.
--
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]
