sam-1112 commented on code in PR #5409:
URL: https://github.com/apache/datafusion-comet/pull/5409#discussion_r3837540189
##########
spark/src/main/scala/org/apache/comet/serde/strings.scala:
##########
@@ -178,29 +182,84 @@ object CometStringReplace
extends CometScalarFunction[StringReplace]("replace")
with NativeOptInAvailable {
+ /**
+ * The DataFusion `replace` kernel matches Spark only for a non-empty search
string. Kernel
+ * compatibility is not enough: `CometLiteral` serializes strings via
`UTF8String.toString`
+ * (malformed UTF-8 becomes U+FFFD), DataFusion evaluates every child before
`replace` (so a
+ * NULL `src` does not skip a throwing replacement), and scalar literals are
broadcast into
+ * Arrow `Utf8` arrays that overflow 32-bit offsets on a large batch.
+ *
+ * The default native path is therefore limited to a plan-time subset that
avoids those
+ * boundaries. Non-default collations stay on the dispatcher.
+ * https://github.com/apache/datafusion-comet/issues/4496
+ */
+ private def nativeSafeSubset(expr: StringReplace): Boolean = {
+ val children = expr.children
+ if (children.length != 3) {
+ return false
+ }
+ val searchIsSafe = children(1) match {
+ case Literal(v: UTF8String, _) => isNativeSafeStringLiteral(v,
allowEmpty = false)
+ case _ => false
+ }
+ val replacementIsSafe = children(2) match {
+ case Literal(null, _) => true
+ case Literal(v: UTF8String, _) => isNativeSafeStringLiteral(v,
allowEmpty = true)
+ case _: Attribute | _: BoundReference => true
+ case _ => false
+ }
+ val utf8BinaryCollation =
+ !children.exists(c => QueryPlanSerde.isStringCollationType(c.dataType))
+ utf8BinaryCollation && searchIsSafe && replacementIsSafe
Review Comment:
You're right — I only gated search and replacement, not source.
`replace(substring(s, 1, CAST(1 / n AS INT)), 'a', 'x')` still went native
because `'a'`/`'x'` looked safe. Spark short-circuits inside `substring` when
`s` is NULL; native evaluates the whole source tree and hits `1/0`. Same hole
for a malformed or huge literal as source, including one nested under `concat`.
Source now uses the same whitelist as replacement: a short, well-formed literal
or a column. `substring` / `concat` / other nested expressions stay on the
dispatcher. I'm not trying to prove an arbitrary source tree is safe. Tests
cover your three cases (substring + ANSI `1/n`, `CAST(X'FF' AS STRING)` as
source, `concat(...)` with a nested malformed literal) plus a large `repeat` as
source. Result matches Spark, EXPLAIN still says 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]
