0lai0 opened a new pull request, #5451:
URL: https://github.com/apache/datafusion-comet/pull/5451
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and
enhancements and this helps us generate change logs for our releases. You can
link an issue to this PR using the GitHub syntax. For example `Closes #123`
indicates that this PR will close issue #123.
-->
Closes #5347
## Rationale for this change
`UnBase64` currently runs through `CometCodegenDispatch`, which pays JVM
codegen + per-row dispatch on the hot path. Every row crosses the JNI boundary
into Spark's generated decoder, which defeats the point of running the
surrounding plan natively.
A native kernel replaces both the crossing and the per-row overhead; the
macro bench (below) shows the JVM path at
1.3–1.8 µs/row across payload shapes, all reducible by 2.4x–3.5x.
`unbase64` is also the last common base64 direction still on the JVM
dispatcher, `spark_base64` (encode) went native in #5350, so this closes out
the pair.
## What changes are included in this PR?
- `native/spark-expr/src/string_funcs/unbase64.rs` — kernel + 17 unit tests.
Ports `java.util.Base64.Decoder.decode0` with `isMIME = true` byte-for-byte,
including the four terminal-shape error messages Spark surfaces as
`IllegalArgumentException`. 256-entry compile-time `BASE64_LUT`; non-alphabet
bytes skipped so `unbase64(base64(x))` round-trips through Spark's default
CRLF-wrapped output. JDK quirk pinned: after padding, JDK's trailing loop tests
`base64[b] < 0`, so extra `=` bytes are silently consumed — `YQ===` decodes to
`a`. An earlier draft rejected these; the regression test is
`extra_trailing_padding_after_valid_pair`.
- `native/spark-expr/benches/unbase64.rs` — criterion bench (short, long
single-line, long CRLF-wrapped, tiny, no-nulls / dense / all nulls,
error-first, scalar).
- `spark/src/main/scala/org/apache/comet/serde/strings.scala` —
`CometUnBase64` is now `CometExpressionSerde with CodegenDispatchFallback`.
`failOnError = true` routes back to the JVM dispatcher via `Unsupported(...)`;
native strict RFC 4648 validation is explicitly a Non-goal on #5347.
- `spark/src/test/resources/sql-tests/expressions/string/unbase64.sql` — SQL
fixture, happy-path only. Error shapes stay in Rust unit tests because Spark
surfaces them as raw `IllegalArgumentException` with no SQL error class.
-
`spark/src/test/scala/org/apache/spark/sql/benchmark/CometUnBase64Benchmark.scala`
— macro bench.
- `docs/source/contributor-guide/expression-audits/string_funcs.md` — audit
entry pinned across 3.4.3 / 3.5.9 / 4.0.4 / 4.1.3 / 4.2.0.
## How are these changes tested?
- **Rust unit tests** (17, all in `unbase64.rs`): empty input,
padded/unpadded, `xx=` / `xxx=` shapes, embedded space, embedded CRLF,
`\r\n`-wrapped round-trip against `spark_base64`, each of the four error
shapes, JDK trailing-`=` quirk, `check_binary_capacity` boundary,
Utf8/LargeUtf8 array dispatch, null propagation, wrong arity, wrong scalar type.
- **SQL fixture** at
`spark/src/test/resources/sql-tests/expressions/string/unbase64.sql`, loaded by
`CometScalarFunctionSuite`. Covers padded, unpadded, empty, NULL, embedded
space, embedded CRLF, plus a round-trip over lengths 3 / 5 / 0 / NULL / 57 / 58
/ 120 (crossing the 57-byte chunk boundary).
- **Local status (darwin arm64):**
- `cargo fmt --check`, `cargo clippy -p datafusion-comet-spark-expr
--tests --benches -- -D warnings` — clean.
- `cargo test -p datafusion-comet-spark-expr` — 17/17 in the new module.
- `mvn -pl spark -am test -Dtest=CometScalarFunctionSuite` — 14/14
including new fixture.
## Benchmarks
Both on darwin arm64, Apple M5, release build.
**Criterion** (`native/spark-expr/benches/unbase64.rs`), 8192 rows/batch,
one null every 10 rows:
| shape | time / batch | throughput |
| ---------------------------------- | ------------: | ----------: |
| short (16 raw B) | 167.9 µs | ~48.8M/s |
| long single-line (200 B) | 1.282 ms | ~6.4M/s |
| long CRLF-wrapped (200 B) | 1.433 ms | ~5.7M/s |
| tiny (1 B / row) | 51.2 µs | ~160M/s |
| scalar literal (200 B) | 183 ns / call | — |
**Macro bench** (`CometUnBase64Benchmark`), Spark vs Comet native, 8192
rows/batch, OpenJDK 17.0.18. Full results:
`spark/benchmarks/CometUnBase64Benchmark-results.txt`.
| shape | Spark (ns/row) | Comet (ns/row) | Speedup |
| ----------------- | -------------: | -------------: | ------: |
| short | 1782.8 | 662.3 | 2.7x |
| long single-line | 1621.0 | 677.5 | 2.4x |
| long CRLF-wrapped | 1597.1 | 646.7 | 2.5x |
| tiny | 1366.0 | 385.1 | 3.5x |
No regressions. Tiny gets the largest speedup because Spark's `doGenCode`
pays per-row codegen/dispatch cost that the native kernel amortises.
--
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]
