andygrove commented on PR #4885: URL: https://github.com/apache/datafusion-comet/pull/4885#issuecomment-5146325053
@mbutrovich both threads addressed in b1acaefbc, with details inline. Short version: **The temporary `String` is gone, and the fix went further than removing it.** I implemented your 57-byte-window suggestion first and benchmarked it — it is correct, but it is **+10.5%** on long chunked input, the case it targets, because the per-call overhead of many small `encode_string` calls exceeds the single bulk encode plus the copy. So instead: `encode_into` now writes to a `fmt::Write` sink with a caller-owned scratch, and since `GenericStringBuilder` implements `fmt::Write`, the array path passes **the builder itself** — the wrapped output lands directly in the value buffer. That removes the per-row temporary *and* the `buf` → builder copy that both the old code and the window version pay: | Benchmark | 57-byte windows | `fmt::Write` into builder | | --- | --- | --- | | short, unchunked | -2.5% | ~0% | | short, chunked | +3.7% | -1.7% | | long, unchunked | -6.1% | -7.2% | | long, chunked | **+10.5%** | **-20.8%** | Your insight about the 57-byte block alignment is documented on `encode_into` along with why it is not used, so it does not get re-derived and re-regressed later. **Capacity is now a real upper bound.** `sum ceil(x_i) <= ceil(sum x_i) + (N - 1)`, so `+ 4 * (N - 1)` bounds the per-row padding while staying O(1) — no reintroduced pass over the offsets. Extracted to `encoded_capacity` with the derivation in a doc comment. **Tests:** an exhaustive sweep over every input length across three line boundaries (asserting equality with encode-then-split, that every non-final line is exactly 76 chars, and that none exceeds it), a capacity upper-bound check over eight array shapes including the all-one-byte worst case under both chunk settings, and an all-tiny-rows array test. Verified: 517 crate unit tests pass, `cargo clippy --all-targets` clean, and the three base64 SQL file tests (`base64.sql`, `base64_unchunked.sql`, `unbase64.sql`) pass against real Spark on 3.5 — those cover the CRLF-wrapped values and the `unbase64` round trip, so the wrapping is validated against Java's MIME encoder end to end and not just against my own reimplementation of 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: [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]
