andygrove opened a new issue, #5347:
URL: https://github.com/apache/datafusion-comet/issues/5347
## Summary
`unbase64` is the last remaining codegen-dispatched member of Comet's base64
family: `base64` has a
native Rust kernel (`spark_base64`,
`native/spark-expr/src/string_funcs/base64.rs`) with a criterion
benchmark, while the decode direction still runs Spark's generated JVM code
inside the Comet
pipeline. Closing this asymmetry removes 5 JVM heap allocations per non-null
row and a double scan of
every input value.
Assessed with the `suggest-native-expression` skill (survey mode over the 58
`Codegen dispatch` rows
in the expression reference). `unbase64` topped the ranking: **compatibility
confidence High**,
**native upside High**.
Note on prior art: #423 ("Support spark unbase64 function") is closed, but
it was about *support*,
which was delivered via codegen dispatch. This issue is about the native
implementation, which does
not exist.
## How it runs today
`object CometUnBase64 extends CometCodegenDispatch[UnBase64]`
(`spark/src/main/scala/org/apache/comet/serde/strings.scala`). A plain
`CometCodegenDispatch` means
the codegen dispatcher is the only path: no native path exists and there is
no `NativeOptInAvailable`
opt-in. `getSupportLevel` is `Compatible()`, so behavior matches Spark
exactly, at the cost of one
JNI round trip per batch plus per-row JVM evaluation.
`StringType` input and `BinaryType` output are both inside
`CometBatchKernelCodegen.isSupportedDataType`,
so there are **no** input shapes that fall back to Spark entirely. The only
fallback trigger is
`spark.comet.exec.scalaUDF.codegen.enabled=false`. The upside here is purely
the dispatch cost, not a
recovered whole-operator fallback.
## Native upside: High
Spark's `doGenCode` for `UnBase64` (unchanged across 3.4.3, 3.5.8, 4.0.1,
4.1.1) emits:
```java
${ev.value} = java.util.Base64.getMimeDecoder().decode($child.toString());
```
`getMimeDecoder()` returns a cached singleton, so that part allocates
nothing. Everything else does.
Per non-null row, counted from the Spark and JDK 17 sources rather than
estimated:
1. `UTF8String.toString()` calls `getBytes()`. In the dispatcher, strings
are read zero-copy through
`UTF8String.fromAddress(null, addr, len)`
(`CometBatchKernelCodegenInput.scala:275`), so `base` is off-heap and
`getBytes()` takes the
copying branch (`UTF8String.java:252-262`). → **1 `byte[]`**
2. `new String(bytes, UTF_8)` (`UTF8String.java:1378-1380`). → **1 `String`
+ 1 internal `byte[]`**
3. `Base64.Decoder.decode(String)` is `decode(src.getBytes(ISO_8859_1))`
(`Base64.java:588-590`).
→ **1 `byte[]`**
4. `decode(byte[])` allocates `new byte[decodedOutLength(...)]`
(`Base64.java:564-571`).
→ **1 `byte[]`**, plus an `Arrays.copyOf` when the estimate overshoots.
That is **5 heap allocations per non-null row, about 41,000 per 8192-row
batch**, none of which are
visible to Comet's native memory pool. On top of that, the MIME decoder
scans each value twice:
`decodedOutLength` pre-scans the whole input to discount non-alphabet bytes
(`Base64.java:712-725`,
comment: "a performance trade-off of pre-scan or Arrays.copyOf") before
`decode0` scans it again.
A native kernel removes all of it with the pattern already documented in
`docs/source/contributor-guide/optimizing_expressions.md` and already used
by `spark_unhex`: a
compile-time 256-entry decode table plus a `BinaryBuilder` preallocated to
`len / 4 * 3`, writing
straight into the output buffer in a single pass with zero per-row heap
allocation. The encode-side
sibling `spark_base64` is the working precedent, including its
`benches/base64.rs`.
Honest limits on this rating:
- The allocation counts above are **derived from source, not measured**. No
JVM benchmark in the repo
covers `base64` or `unbase64`, so there is no baseline to quote.
Establishing one is part of the
work.
- Workload presence is **unmeasured**. `grep` over `benchmarks/tpc/queries/`
finds zero uses of
`unbase64` in TPC-H or TPC-DS. The claim that base64 decoding is common in
ETL pipelines (decoding
embedded payloads, Kafka values, log fields) is a judgment call, not a
measurement.
## Compatibility assessment: High
### Spark versions
Semantics are **identical** across 3.4.3, 3.5.8, 4.0.1, and 4.1.1. The only
differences are
non-behavioral:
- 4.0.1: the `NullIntolerant` trait becomes `override def nullIntolerant:
Boolean = true`, and
`inputTypes` widens from `Seq(StringType)` to
`Seq(StringTypeWithCollation(supportsTrimCollation = true))`.
- 4.1.1: adds `contextIndependentFoldable`.
No shims are needed for the native kernel.
### Hazard checklist
| Hazard | Applies? |
| --- | --- |
| JVM formatting APIs (`DecimalFormat`, `String.format`, date formatters) |
No |
| `java.util.regex` features Rust's `regex` lacks | No |
| Collation on Spark 4.0+ | **No, cleared with reasoning.** `inputTypes`
accepts collated strings, but the implementation never routes through
`CollationSupport`. Base64 decoding is byte-level and collation-independent, so
there is no divergence to guard. |
| `BigDecimal` / `MathContext` | No |
| Timezone and calendar | No |
| ANSI error class and message parity | **Yes, bounded.** See below. |
| Float/double text conversion, `StrictMath` | No |
| Raw byte semantics of `UTF8String` (invalid UTF-8, #4764) | **No, cleared
with reasoning.** Spark itself decodes via `toString()`, which replaces
ill-formed sequences with `U+FFFD`; `U+FFFD` is not in the base64 alphabet, so
the MIME decoder skips it. Comet's ingress replacement produces the same
character stream. |
| Lambda bodies / arbitrary Catalyst trees | No |
| JVM session state beyond a scalar config | No |
| Nondeterminism or ordering sensitivity | No |
| Cross-version behavioral differences | No (see above) |
### The one real hazard: the decoder is the JDK MIME decoder, not a standard
base64 engine
This is the crux, and it is where a naive implementation would ship a
correctness bug.
`java.util.Base64.getMimeDecoder()` **silently skips every byte outside the
base64 alphabet** and
has four enumerable error conditions (JDK 17 `Base64.java`):
1. `"Input byte array has wrong 4-byte ending unit"` — malformed padding
(`=`, `xx=y`, `xx=` with a
missing second `=`).
2. `"Last unit does not have enough valid bits"` — a dangling single
character.
3. `"Input byte array has incorrect ending byte at N"` — an alphabet
character appearing after
padding.
4. `"Input byte[] should at least have 2 bytes for base64 bytes"` — from
`decodedOutLength`.
Every one of these is mechanically reproducible from the JDK source, which
is why the rating is High
rather than Medium. Comet has already accepted a port of the mirror-image
MIME **encoder** semantics
in `spark_base64` (76-char lines, CRLF, no trailing separator), so this is
consistent with existing
precedent. Error parity mechanics are covered by
`docs/source/contributor-guide/sql_error_propagation.md`.
### Upstream is NOT usable as-is
`datafusion-spark` 54.1.0 (the version Comet depends on) has
`SparkUnBase64`, but it is a `simplify`
wrapper that lowers to `decode(bin, 'base64pad')`, and `decode`'s base64
path uses
`datafusion-functions`' `BASE64_ENGINE`: `base64::alphabet::STANDARD` with
`DecodePaddingMode::Indifferent`
(`datafusion-functions-54.1.0/src/encoding/inner.rs:45-51`). That engine
**errors** on any
non-alphabet byte instead of skipping it, which diverges from Spark:
| Input | Spark (`getMimeDecoder`) | `datafusion-spark::SparkUnBase64` |
| --- | --- | --- |
| `unbase64('YW Jj')` | `abc` (space skipped) | error: `Failed to decode
from base64: Invalid byte 32` |
| `unbase64('YWJj?')` | `abc` (`?` skipped) | error |
| `unbase64(base64(x))` where `x` is longer than 57 bytes | round-trips |
error: Spark's `base64` CRLF-chunks at 76 chars by default
(`spark.sql.chunkBase64String.enabled=true`), and Comet's own `spark_base64`
does the same, so the encoded value contains `\r\n` |
The third row is the important one: wiring the upstream function would break
round-tripping Comet's
own `base64` output, which is the most common way `unbase64` is used. Worth
reporting upstream
separately.
## Proposed approach
Implement a native kernel rather than wiring upstream. See the
`implement-comet-expression` skill.
1. Add `spark_unbase64` under `native/spark-expr/src/string_funcs/`, next to
`base64.rs`. Port the
JDK MIME decoder rules: skip non-alphabet bytes, handle the four error
conditions above, single
pass into a `BinaryBuilder` preallocated to `len / 4 * 3`, 256-entry
decode table.
2. Wire it in `native/spark-expr/src/comet_scalar_funcs.rs` alongside the
existing `"base64"` arm.
3. Change `CometUnBase64` from `CometCodegenDispatch[UnBase64]` to a
`CometExpressionSerde[UnBase64]`
mixing in `CodegenDispatchFallback`, with `getSupportLevel` returning
`Unsupported(Some(...))` for `failOnError = true` so those cases keep
running on the dispatcher
(see scope below).
4. Add a criterion benchmark `native/spark-expr/benches/unbase64.rs`
covering the shapes required by
`optimizing_expressions.md`: no nulls / sparse nulls / dense nulls, short
/ long values, padded /
unpadded, clean / with skipped characters (CRLF-chunked input), and the
error path.
5. Add Comet SQL Tests at
`spark/src/test/resources/sql-tests/expressions/string_funcs/unbase64.sql`.
### Scope
`failOnError = true` is reachable only from `to_binary(x, 'base64')` and
`try_to_binary`, and it
needs Spark's separate, stricter `UnBase64.isValidBase64` validator (RFC
4648-ish: whitespace
skipped, padding must conclude the string, last group rules). `failOnError`
is a plain boolean field
on the expression node, so it is fully detectable at plan time. Keep those
cases on
`CodegenDispatchFallback` in the first PR and treat native validation as a
follow-up.
### Non-goals
- Native `failOnError = true` (`to_binary` / `try_to_binary`), per above.
- Any collation work. `unbase64` has no collation-dependent behavior.
- Changing `base64` (encode), which is already native.
## Acceptance criteria
- Output is bit-identical to the codegen-dispatch path for every input,
including null placement and
the four `IllegalArgumentException` conditions with matching messages.
- Round-tripping Comet's own `base64` output works for values long enough to
be CRLF-chunked.
- Criterion benchmark committed, covering the shapes listed above, with no
regression on any shape.
- Comet SQL Tests cover column and literal arguments, `NULL`, empty string,
padded and unpadded
input, skipped characters, embedded CRLF, and each error condition.
- The `unbase64` entry in
`docs/source/contributor-guide/expression-audits/string_funcs.md` is
updated when the work lands.
---
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).
The assessment is recorded in
`docs/source/contributor-guide/expression-audits/string_funcs.md` under `##
unbase64`.
--
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]
