andygrove opened a new pull request, #5348: URL: https://github.com/apache/datafusion-comet/pull/5348
## Which issue does this PR close? N/A. This adds contributor tooling rather than fixing a filed issue. It supports the roadmap item [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). ## Rationale for this change 58 Spark expressions currently run through the JVM codegen dispatcher. Deciding which of them is worth a native Rust implementation is a recurring judgment call, and it is easy to get wrong in both directions: proposing a native kernel that cannot match Spark, or spending a week on a rewrite whose only saving is the per-batch JNI round trip. The two questions that decide it are always the same: 1. Can a native path be **100% Spark-compatible**? The dispatcher it would replace is byte-exact by construction, so replacing it with an approximate path is a regression, not an optimization. 2. Does it deliver a real **throughput or allocation** win over codegen dispatch? The dispatcher already runs in-pipeline, reads Arrow vectors directly, reads strings zero-copy, and compiles its kernel once per `(expression, schema)` pair. "Native is faster than JVM" is not an argument on its own. Today those questions get re-answered from scratch every time, and the answers are not written down, so a candidate that was already ruled out can be proposed again. This skill makes the assessment repeatable and, more importantly, **cumulative**: every verdict lands in the per-expression audit log, including the disqualifications, so the pool shrinks over time instead of being re-walked. ## What changes are included in this PR? - **`.claude/skills/suggest-native-expression/SKILL.md`**, a new skill that: - builds the candidate pool from the `Codegen dispatch` rows of the expression reference, excluding families the project has ruled out, expressions already assessed in the audit log, and expressions with an existing issue; - scores **compatibility confidence** against a 12-row hazard checklist (JVM formatting APIs, `java.util.regex` features Rust's `regex` lacks, Spark 4 collation, `BigDecimal`/`MathContext`, tzdb and hybrid-calendar rebase, ANSI error-class parity, `Double.toString`, invalid-UTF-8 byte semantics, lambda bodies, JVM session state, nondeterminism, cross-version divergence), with a `Low` rating as a hard disqualifier; - scores **native upside** from what the dispatcher actually costs, and spells out what it does *not* cost so the upside cannot be overstated. The rating has to rest on per-row heap allocation, hoistable per-row work, a named technique from `optimizing_expressions.md`, nested-type wrapper allocation, a recovered whole-operator Spark fallback, or workload presence; - requires an empirical calibration step when either axis lands on `Medium`, since that is where a static read is least trustworthy; - maps the two ratings to `Recommended` / `Deferred` / `Disqualified`, and requires a specific, re-checkable blocker for anything not recommended; - files exactly one issue per run and records the verdict in the audit log for **every** verdict, including disqualifications. - **`docs/source/contributor-guide/expression-audits/index.md`**: documents the new `Native candidate (assessed ...)` line as a third kind of audit entry, alongside correctness and performance audits. - **`docs/source/contributor-guide/expression-audits/string_funcs.md`**: the first assessment, recorded for `unbase64`. ## Example of what the skill produces Running the skill in survey mode over the pool produced **[#5347: Implement `unbase64` natively instead of JVM codegen dispatch](https://github.com/apache/datafusion-comet/issues/5347)**, which is a fair sample of the output quality this is aiming for: - **Ranking.** 58 candidates. `unbase64` topped it because `base64` already has a native kernel (`spark_base64`) with a criterion benchmark while the decode direction does not, so the family is asymmetric for no stated reason. - **Upside, counted rather than asserted.** 5 JVM heap allocations per non-null row, traced through `UTF8String.getBytes()` (which copies, because the dispatcher hands it an off-heap `UTF8String.fromAddress`), `new String(bytes, UTF_8)`, `decode(String)`'s `getBytes(ISO_8859_1)`, and the output `byte[]`. About 41k allocations per 8192-row batch, plus a double scan of every value because MIME's `decodedOutLength` pre-scans to size the output. All removable with the `spark_unhex` pattern. - **Compatibility, checked across versions.** Semantics identical on 3.4.3, 3.5.8, 4.0.1, and 4.1.1, so no shims. Collation and invalid-UTF-8 are explicitly cleared **with reasoning** rather than waved off: `unbase64` never routes through `CollationSupport`, and Spark itself decodes through `toString()`, so Comet's ingress replacement produces the same character stream. - **The finding that makes the issue worth filing.** `datafusion-spark::SparkUnBase64` exists, so the obvious move is to wire it up. Reading it shows it lowers to `decode(bin, 'base64pad')`, whose strict engine errors on the non-alphabet bytes Spark's MIME decoder skips. Since Spark's `base64` CRLF-chunks at 76 characters by default, wiring the upstream function would **break round-tripping Comet's own `base64` output**. The skill's "read it and diff the semantics before crediting it" rule is what caught that. - **Honest limits.** The allocation counts are labelled as derived from source, not measured, and workload presence is labelled unmeasured (zero hits in `benchmarks/tpc/queries/`). The corresponding audit-log line records the same verdict in one scannable bullet, so the next run sees `unbase64` is taken without re-reading any of it. ## How are these changes tested? Docs and skill definition only, so there is nothing executable to test. Validation was to run the skill end to end, which is what produced #5347 and the `unbase64` audit-log entry. Every command in the skill was executed during that run, and the four issues it cites (#4496, #4506, #4654, #4764) were verified open. `npx prettier` was run over the changed markdown. -- 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]
