sunchao commented on code in PR #5409:
URL: https://github.com/apache/datafusion-comet/pull/5409#discussion_r3837642570
##########
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:
[P2] Account for batches expanded by native `explode`
This still reproduces on `db8e31d2cb9d1d63798fb26cbf7318920aecd394`. The
literal cap uses `spark.comet.batchSize`, but native `explode` can emit a
larger batch without rebatching it.
With an 8,192-row Parquet input containing only short strings `'a'`/`'b'`,
the default batch size of 8,192, and
`spark.comet.expression.StringReplace.allowIncompatible=false`:
```sql
SELECT replace(e, 'notfound', repeat('x', 131072))
FROM t
LATERAL VIEW explode(array(s, s)) a AS e
```
The 131,072-byte replacement passes the 262,143-byte cap. However, `explode`
produces one 16,384-row batch, so broadcasting the unused replacement would
require 2,147,483,648 bytes and fails with `CometNativeException: native panic:
offset overflow`. The exact-base dispatcher returns all 16,384 unchanged
strings. Both A/B plans contain `CometProject` and `CometExplode`, with only
the head routing `replace` natively.
Could this use scalar-aware handling or an enforced runtime batch bound
before enabling these calls by default? Please add a regression after
`explode`, since the one-row oversized-literal test checks routing but cannot
establish this bound.
Verification used freshly compiled base
(`367ca64be4e62dd10dc4c932de9ca738355a45fa`) and head `strings.scala` on the
same cached Spark 4.0.4/Comet support runtime. This was focused A/B validation,
not a fresh full-head Maven/native build.
--
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]
