sunchao commented on code in PR #5409:
URL: https://github.com/apache/datafusion-comet/pull/5409#discussion_r3836632726
##########
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:
[P2] Check the source subtree before declaring the call native-safe
Could this eligibility check cover `children(0)` as well? Under ANSI, with
Parquet rows `(s=NULL,n=0)` and `(s='a',n=1)`, `replace(substring(s, 1, CAST(1
/ n AS INT)), 'a', 'x')` still passes this guard and throws `DIVIDE_BY_ZERO`.
The base dispatcher returns NULL and `'x'`, because Spark's `Substring` skips
the length expression on the NULL row. Native conversion recursively evaluates
that unchecked source subtree instead.
Source literals bypass the new byte/size checks too. With a replacement
column `r='x'`, `replace(CAST(X'FF' AS STRING), 'a', r)` changes raw `FF` into
`EF BF BD`. A 256 KiB folded source with an empty replacement column still
overflows when broadcast to 8,192 rows. These all reproduced in the base/head
A/B with `allowIncompatible=false`. Checking only a top-level source literal
would miss malformed literal descendants such as `concat(CAST(X'FF' AS STRING),
r)`, so please retain dispatcher routing unless the source subtree is safe too.
--
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]
