You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "andygrove (via GitHub)" <gi...@apache.org> on 2026/05/30 16:34:51 UTC
[PR] fix: route StringReplace through codegen dispatcher to fix empty-search divergence [datafusion-comet]
andygrove opened a new pull request, #4537:
URL: https://github.com/apache/datafusion-comet/pull/4537
## Which issue does this PR close?
Closes #4497.
## Rationale for this change
`replace(str, search, replacement)` diverges from Spark when the search string is empty. Spark short-circuits and returns the source unchanged (`UTF8String.replace` checks `search.numBytes == 0`). DataFusion's `replace` instead inserts the replacement between every character and at both boundaries, producing e.g. `replace('hello', '', 'x')` -> `xhxexlxlxox`. The bug applies to both literal empty searches and column-valued empty strings at runtime.
## What changes are included in this PR?
- New `CometStringReplace` serde in `spark/src/main/scala/org/apache/comet/serde/strings.scala`. By default it routes through `CometScalaUDF.emitJvmCodegenDispatch` so Spark's own `doGenCode` runs inside the Comet pipeline. Users can opt back into the native (divergent) path with `spark.comet.expression.StringReplace.allowIncompatible=true`. The pattern mirrors `CometInitCap`.
- `QueryPlanSerde` now wires `StringReplace` to `CometStringReplace` instead of the inline `CometScalarFunction("replace")`.
- `string_replace.sql` removes the `query ignore(#3344)` directive on the empty-literal case, adds rows for empty-source / NULL-source / NULL-replacement, and adds a row `('hello', '', 'x')` to the test table to exercise the column-valued empty-search case.
## Are these changes tested?
Yes. `CometSqlFileTestSuite` runs the updated `string_replace.sql` and passes on Spark 3.5. Compile verified for Spark 3.4 (Java 11), Spark 3.5 (Java 11), and Spark 4.0 (Java 17).
## Are there any user-facing changes?
The default behavior of `replace` becomes Spark-compatible for all empty-search cases. Plans that previously executed `replace` natively now route through the JVM codegen dispatcher when `spark.comet.exec.scalaUDF.codegen.enabled=true` (the default), which is slightly slower than the native path but matches Spark exactly. The native path remains available behind `spark.comet.expression.StringReplace.allowIncompatible=true`.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: route StringReplace through codegen dispatcher to fix empty-search divergence [datafusion-comet]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #4537:
URL: https://github.com/apache/datafusion-comet/pull/4537#discussion_r3341674022
##########
spark/src/main/scala/org/apache/comet/serde/strings.scala:
##########
@@ -102,6 +102,27 @@ object CometInitCap extends CometScalarFunction[InitCap]("initcap") {
}
}
+object CometStringReplace extends CometScalarFunction[StringReplace]("replace") {
+
+ override def getSupportLevel(expr: StringReplace): SupportLevel = Compatible()
+
+ override def convert(
+ expr: StringReplace,
+ inputs: Seq[Attribute],
+ binding: Boolean): Option[Expr] = {
+ if (CometConf.isExprAllowIncompat(getExprConfigName(expr))) {
+ // Native path: faster, but DataFusion's `replace` diverges from Spark when the search
+ // string is empty — it inserts the replacement between every character and at both
+ // boundaries, while Spark short-circuits and returns the source unchanged. Opt-in.
+ super.convert(expr, inputs, binding)
+ } else {
+ // Default: route through the codegen dispatcher so Spark's own doGenCode handles the
+ // empty-search edge case. https://github.com/apache/datafusion-comet/issues/4497
Review Comment:
Do we want to link to closed issues in the code (this PR closes it). I think we should make sure the code is self-documenting, with comments explaining why code exists if it's non-obvious. Also comments that say things like "default" are a code smell if that ever changes.
##########
spark/src/main/scala/org/apache/comet/serde/strings.scala:
##########
@@ -102,6 +102,27 @@ object CometInitCap extends CometScalarFunction[InitCap]("initcap") {
}
}
+object CometStringReplace extends CometScalarFunction[StringReplace]("replace") {
+
+ override def getSupportLevel(expr: StringReplace): SupportLevel = Compatible()
+
+ override def convert(
+ expr: StringReplace,
+ inputs: Seq[Attribute],
+ binding: Boolean): Option[Expr] = {
+ if (CometConf.isExprAllowIncompat(getExprConfigName(expr))) {
+ // Native path: faster, but DataFusion's `replace` diverges from Spark when the search
Review Comment:
Is this already documented somewhere else? This doesn't seem like the right place for it.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: route StringReplace through codegen dispatcher to fix empty-search divergence [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove merged PR #4537:
URL: https://github.com/apache/datafusion-comet/pull/4537
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org