sam-1112 opened a new pull request, #5409:
URL: https://github.com/apache/datafusion-comet/pull/5409
## Which issue does this PR close?
Closes #5354.
## Rationale for this change
DataFusion `replace` only diverges from Spark when `search` is empty: Spark
returns `src` unchanged, while DataFusion inserts `replace` between every
character. For the common case—a non-empty `UTF8_BINARY` literal—the native
kernel is Spark compatible, so Comet can use it by default instead of the JVM
codegen dispatcher. Routing is decided by `search` only. The replacement
argument is not required to be a literal, so `replace(col, 'literal',
replacementColumn)` also runs natively. Empty literal search, non-literal
search, and non-default collations remain on the dispatcher.
`spark.comet.expression.StringReplace.allowIncompatible` is unchanged and still
opts the remaining cases into native execution.
This PR does not change the native kernel. Collation support remains out of
scope (#4496).
## What changes are included in this PR?
- `CometStringReplace` uses native DataFusion `replace` by default when
`search` is a non-empty `UTF8_BINARY` literal.
- Empty literal search, non-literal search, and non-default collations
continue to use the JVM codegen dispatcher.
- Updated the `## replace` section in
`docs/source/contributor-guide/expression-audits/string_funcs.md`.
- Added SQL coverage for overlapping replacements and multi-byte UTF-8 on
the native path.
- Added routing coverage for native versus dispatcher execution.
## How are these changes tested?
### SQL tests
File:
`spark/src/test/resources/sql-tests/expressions/string/string_replace.sql`
Existing unchanged coverage includes:
- Empty search: `replace('hello', '', 'x')` and related NULL/empty variants
- Empty source: `('', 'a', 'b')` in the table scan
- No match: `('hello', 'xyz', 'abc')`
- Single match: `('hello world', 'world', 'there')`
- Multiple matches: `('aaa', 'a', 'bb')`
This PR adds column-source, literal-search cases that take the native path:
- Overlapping candidates: `replace(s, 'aa', 'x')` on `'aaaa'` → `'xx'`
- Multi-byte UTF-8 values, including `'你好你好'` and `'😀a😀'`
### Routing tests
`CometCodegenSuite` asserts the following routing behavior:
- Non-empty literal search → native
- Empty literal search → JVM codegen dispatcher
- Column search → JVM codegen dispatcher
- Non-default collation on Spark 4+ → JVM codegen dispatcher
The non-default-collation assertion is gated by `isSpark40Plus`.
### Commands executed
The SQL file and routing tests were run on Spark 4.1, 3.5, and 3.4:
- `./mvnw test -Dtest=none -Dsuites="org.apache.comet.CometSqlFileTestSuite
string_replace"`
- `./mvnw test -Dtest=none -Dsuites="org.apache.comet.CometCodegenSuite
replace routes native"`
Spark 4.1 was tested with JDK 17. Spark 3.5 and 3.4 were tested with JDK 11.
## Performance trade-off
Native execution is **not faster** in this microbenchmark. It uses
**2.5–2.9× as much CPU time** as the dispatcher; at 100% match density, `474.6
/ 166.2 ≈ 2.86×`.
This PR therefore does not claim a throughput improvement. It trades CPU
throughput for reduced dispatcher-side JVM heap churn on a scan-bound path.
Reviewer feedback on whether that trade-off justifies changing the default is
especially welcome.
Benchmark query:
```sql
SELECT replace(c1, '123', 'ab')
FROM parquetV1Table
```
Rows: **1,048,576**
All timings are in **ns/row; lower is better**.
| Match density | Spark (ns/row) | Comet dispatcher (ns/row) | Comet native
(ns/row) | Native / dispatcher |
|--------------:|---------------:|--------------------------:|----------------------:|--------------------:|
| 0% | 34.1 | 153.1 | 395.9
| 2.59× |
| 10% | 48.2 | 162.2 | 407.5
| 2.51× |
| 100% | 71.6 | 166.2 | 474.6
| 2.86× |
The dispatcher is estimated to allocate approximately **80–136 bytes/row**
for this path, based on the JVM-heap `UTF8String` analysis in #5354. This
allocation was **not re-measured in this benchmark run**, and Spark
`Benchmark`'s allocated-bytes column was not recorded.
The native path avoids those dispatcher-side per-row JVM string allocations.
### Benchmark environment and methodology
- Spark **4.1.2**, using Comet's default profile at the time of measurement
- Apple M4
- OpenJDK 17
- Release native library built with `make release`
- Spark `Benchmark` defaults:
- Warmup: **2 seconds**
- `minNumIters`: **2**
- `minTime`: **2 seconds**
After this change, a non-empty literal search always takes the native path.
Measuring the dispatcher arm therefore required temporarily forcing
`nativeSafeSearchSubset` to `false` in a local benchmark harness. That harness
change is not included in this PR.
--
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]
