andygrove commented on PR #5350: URL: https://github.com/apache/datafusion-comet/pull/5350#issuecomment-5286787510
Fifth run: **[#5354: Run `replace` natively by default when the search string is a non-empty literal](https://github.com/apache/datafusion-comet/issues/5354)**, recorded under `## replace` in `string_funcs.md`. **Correction to what I wrote last run.** In the [previous comment](https://github.com/apache/datafusion-comet/pull/5350#issuecomment-5286725412) I said `replace`'s allocation saving "only materialises on matching rows", because `UTF8String.replace` returns `this` on a non-match. That is wrong, and measuring it is what caught it. The dispatcher hands the expression an **off-heap** `UTF8String.fromAddress`, so on a non-match the returned string's base is `null` rather than a `byte[]`, and the kernel's output write takes the `getBytes()` copy branch: ```java 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(); // allocates, even for a pass-through row output.setSafe(i, utfArr_0, 0, utfArr_0.length); } ``` Allocated bytes per row, via `getThreadAllocatedBytes` over 8192 off-heap-backed 64-byte values, including that output 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 | The same measurement with heap-backed inputs reports **0.0 bytes/row at 0% density**, which is the number my earlier reasoning implicitly assumed. That gap is the whole correction: allocation is unconditional, 80 to 136 bytes/row, roughly 650 KB to 1.1 MB per batch. The rest of the assessment: - **Compatibility High**, and the narrowest gap in the pool: the entire divergence is "empty search string" (Spark short-circuits and returns the input, DataFusion inserts the replacement between every character). Decidable at plan time when the search argument is a `Literal`. Semantics identical across all five pinned versions for `UTF8_BINARY`, since Spark 4.x's `CollationSupport.StringReplace.execBinary` is the same `src.replace(...)` call as 3.x. - **The guard is an expression property, not a data property**, so unlike `upper`/`lower` in #5353 it needs no per-batch fallback mechanism and is expressible today. - **No native code.** The kernel exists; this is a `getSupportLevel` change plus tests. - **What I did not prove:** the wall-clock win. Time per row is flat at 170 to 195 ns and barely moves with density, so this path is scan-bound rather than allocation-bound. Whether Rust's substring search beats `UTF8String.find`'s byte loop needs a release build of the native lib, so it is in the acceptance criteria with an explicit instruction to report the number even if it is unflattering. Also worth noting for anyone reading the history: #3344, one of the two closed issues on this divergence, **states it backwards** (it claims Spark produces `xhxexlxlxox`). #4497 has it right. I flagged that in the issue so nobody re-derives the semantics from the wrong one. Five assessments now, across three category pages: three Recommended, one Recommended-and-scoped, one Deferred. -- 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]
