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:
Thanks for catching this. I updated the eligibility check so `children(0)`
uses the same conservative whitelist as the replacement argument: only a column
reference, `NULL`, or a short byte-preserving UTF-8 literal is accepted.
Nested source expressions now remain on the dispatcher, so the `substring`
short-circuit case and malformed literals hidden under `concat` are not
converted recursively to native execution.
I also added regression coverage for:
- the ANSI `substring` / `DIVIDE_BY_ZERO` case,
- a malformed source literal,
- a malformed literal nested under `concat`, and
- an oversized folded source literal.
These cases now preserve Spark results and use the JVM codegen dispatcher
with `allowIncompatible=false`.
--
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]
