andygrove opened a new issue, #5354:
URL: https://github.com/apache/datafusion-comet/issues/5354
## Summary
`replace` has a native kernel already (DataFusion `replace`), gated off
behind
`spark.comet.expression.StringReplace.allowIncompatible`. Its entire
incompatibility is **one input
condition**: Spark returns the input unchanged when the search string is
empty, DataFusion inserts the
replacement between every character.
```
SELECT replace('hello', '', 'x') -- Spark: hello DataFusion:
xhxexlxlxox
```
That condition is a plain emptiness check on an argument, so when the search
argument is a `Literal`
(the overwhelmingly common shape) it is decidable at plan time. This is the
narrowest compatibility gap
of any candidate assessed so far, and closing it needs no native code.
Assessed with the `suggest-native-expression` skill: **compatibility
confidence High**, **native upside
Medium to High, measured**. Verdict: Recommended.
Prior art: [#3344](https://github.com/apache/datafusion-comet/issues/3344)
and
[#4497](https://github.com/apache/datafusion-comet/issues/4497) are both
closed. They reported the
divergence as a bug and it was fixed by routing `replace` through the
dispatcher, which is the current
state. Neither covers making the native path the default for the safe
subset. (Note for the record:
#3344 states the divergence backwards, claiming Spark produces
`xhxexlxlxox`. #4497 has it right.)
## How it runs today
`object CometStringReplace extends
CometScalarFunction[StringReplace]("replace") with NativeOptInAvailable`
(`spark/src/main/scala/org/apache/comet/serde/strings.scala:179`).
`getSupportLevel` returns `Compatible(nativeOptIn = ...)` unconditionally
when the user has not opted
in, so **every** `replace` runs on the dispatcher, including the vast
majority whose search string is a
non-empty literal and for which the native kernel is already exactly correct.
`getIncompatibleReasons()` is "Produces different results from Spark when
the search string is empty",
which already names the whole gap.
## Native upside: Medium to High (measured)
Two measurements, both on this branch.
**1. The emitted kernel.** Dumped via
`CometBatchKernelCodegen.generateSource` for
`StringReplace(BoundReference(0, StringType), Literal("XYZ"),
Literal("QQ"))`:
```java
value_0 = CollationSupport.StringReplace.execBinary(value_1, references[0],
references[1]);
...
Object utfBase_0 = value_0.getBaseObject();
if (utfBase_0 instanceof byte[]) {
output.setSafe(i, (byte[]) utfBase_0, ..., utfLen_0);
} else {
byte[] utfArr_0 = value_0.getBytes(); // allocation + copy
output.setSafe(i, utfArr_0, 0, utfArr_0.length);
}
```
The important subtlety is in that output branch. `UTF8String.replace`
returns `this` when there is no
match, and in the dispatcher the input is off-heap backed
(`UTF8String.fromAddress`, so `base` is
`null`, not a `byte[]`). Non-matching rows therefore take the **`getBytes()`
branch and allocate a copy
anyway**. Reading `UTF8String.replace` in isolation suggests non-matching
rows are free. They are not.
**2. Allocated bytes per row**, measured with
`com.sun.management.ThreadMXBean.getThreadAllocatedBytes` over 8192
off-heap-backed 64-byte values,
100 iterations after 200 warmup passes, including the kernel's output-write
branch:
| match density | bytes/row | ns/row |
| --- | --- | --- |
| 0% | 80.0 | 171.0 |
| 10% | 85.6 | 195.8 |
| 50% | 108.0 | 170.5 |
| 100% | 136.0 | 182.4 |
For contrast, the same measurement with heap-backed inputs (which is *not*
what the dispatcher
provides) reports 0.0 bytes/row at 0% density. That difference is exactly
the output-side copy above,
and it is why the off-heap variant is the one to trust.
So the allocation saving is **unconditional**, 80 to 136 bytes per row,
roughly 650 KB to 1.1 MB of JVM
heap churn per 8192-row batch, none of it visible to Comet's native memory
pool. A native kernel writes
straight into the Arrow output buffer, and for the pass-through case can
copy buffer to buffer.
**What is not established:** the wall-clock win. Time per row is flat at
roughly 170 to 195 ns and
barely moves with density, so this path is **scan-bound, not
allocation-bound**. Whether native is
faster depends on whether Rust's substring search beats `UTF8String.find`'s
byte-at-a-time loop, which
is plausible (memchr-accelerated) but unmeasured here because it needs a
release build of the native
library. Both paths already exist, so this is cheap for the implementer: run
`CometStringExpressionBenchmark`
with and without
`spark.comet.expression.StringReplace.allowIncompatible=true` at several match
densities. It is in the acceptance criteria below rather than asserted here.
## Compatibility: High
### Spark versions
Identical across 3.4.3, 3.5.9, 4.0.4, 4.1.3, and 4.2.0 for the default
collation. 3.4.3 and 3.5.9 are
byte-identical; 4.0.4 adds `NullIntolerant` becoming an override, a
`collationId` field, and routes
through `CollationSupport.StringReplace.exec`, whose `isUtf8BinaryType`
branch is
`execBinary(src, search, replace) = src.replace(search, replace)`, i.e. the
same call as 3.x. 4.0.4,
4.1.3, and 4.2.0 are byte-identical to each other. No shims needed.
### Hazard checklist
| Hazard | Applies? |
| --- | --- |
| Empty search string | **Yes, and this is the whole gap.** Spark
short-circuits on `search.numBytes == 0` (`UTF8String.java`) and returns the
input. DataFusion inserts between every character. Decidable at plan time for a
literal search argument. |
| Collation on Spark 4.0+ | **Yes.** Non-UTF8_BINARY collations route to
`execLowercase` / `execICU`, which the native path does not implement
([#4496](https://github.com/apache/datafusion-comet/issues/4496)). Out of
subset, and the collation is on the expression's type, so this is plan-time
detectable. |
| Empty **source** string | No. Spark returns the input; DataFusion's
replace over an empty haystack also produces empty. Same result. |
| JVM formatting / locale / ICU | No. Byte-level search and splice, no
locale involvement. |
| Invalid UTF-8 | No more than any other string expression; the search and
replace are both byte sequences and Comet's ingress policy already applies. |
| Regex features, decimal, timezone, ANSI errors, lambdas, nondeterminism,
host state | No |
Unlike `upper`/`lower`
([#5353](https://github.com/apache/datafusion-comet/issues/5353)), the guard
here
is an **expression** property rather than a **data** property, so it needs
no per-batch fallback
mechanism and is expressible in today's architecture.
## Proposed approach
No native code. This is a `getSupportLevel` change plus tests.
1. In `CometStringReplace.getSupportLevel`, return `Compatible(None)` when
the search argument is a
non-empty `Literal` **and** the collation is UTF8_BINARY. That case takes
the native path by default.
2. Everything else keeps today's behaviour exactly: an empty literal search,
a non-literal search, or a
non-default collation returns `Compatible(nativeOptIn = ...)` and rides
the dispatcher.
3. Keep `getIncompatibleReasons()` as is, since it still describes the
opt-in path for the cases that
stay on the dispatcher, and add a compatible note documenting the new
default.
4. Consider a follow-up for the non-literal search case: a native kernel
could implement Spark's
short-circuit directly (`if search.is_empty() { return src }`), which
would make the whole expression
compatible rather than just the literal subset. That is a small kernel
change in
`datafusion-comet-spark-expr` (or upstream), and it would be the cleaner
end state. Filing this as
step 4 rather than step 1 because the plan-time guard delivers most of
the value with no native
change at all.
### Non-goals
- Collation support. Non-default collations stay on the dispatcher.
- Changing the `allowIncompatible` config surface.
## Acceptance criteria
- `replace(col, 'nonempty', r)` runs natively by default, asserted via the
absence of the expression
from the `[COMET-INFO: JVM codegen dispatcher: ...]` EXPLAIN segment.
- `replace(col, '', r)`, a non-literal search, and non-default collations
demonstrably still route to
the dispatcher, each with a test.
- A Comet SQL Test covering empty search, empty source, no-match, single
match, multiple matches,
overlapping candidates, and multi-byte UTF-8 values.
- `CometStringExpressionBenchmark` numbers, dispatcher versus native, at 0%,
10%, and 100% match
density, in the PR description. If native turns out **not** to be faster
despite the allocation
saving, say so in the PR and let reviewers decide whether the reduced heap
churn alone justifies the
change.
- The `replace` entry in
`docs/source/contributor-guide/expression-audits/string_funcs.md` updated.
---
Filed by the `suggest-native-expression` skill. Motivation:
[Native Coverage for Codegen-Dispatched
Expressions](https://github.com/apache/datafusion-comet/blob/main/docs/source/contributor-guide/roadmap.md#native-coverage-for-codegen-dispatched-expressions).
Assessment recorded in
`docs/source/contributor-guide/expression-audits/string_funcs.md` under
`## replace`. Earlier runs of the same skill produced #5347, #5349, #5351,
and #5353.
--
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]
