sam-1112 commented on code in PR #5409:
URL: https://github.com/apache/datafusion-comet/pull/5409#discussion_r3834771955
##########
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)) {
Review Comment:
Makes sense — we shouldn't native this just because the search is a
non-empty literal. I didn't change the kernel to be scalar-aware. Until we can
apply a big Utf8 scalar without broadcasting it, these stay on the dispatcher.
Literals are capped at `Int.MaxValue / spark.comet.batchSize` (262,143 bytes at
the default 8,192). `repeat('x', 262144)` is over that if it folds, and if it
doesn't fold it isn't a literal/column anyway, so either way we dispatch. Large
search literals get the same cap.
Test: `replace(s, 'notfound', repeat('x', 262144))` returns the original
strings and stays on the dispatcher.
--
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]
