suggest-native-expression

<!-- Licensed to the Apache Software Foundation (ASF) under one or more contributor license agreements. See the NOTICE file distributed with this work for additional information regarding copyright ownership. The ASF licenses this file to you under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0 Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. --> Assess whether a Spark expression that Comet runs through the JVM codegen dispatcher is worth implementing natively in Rust, and propose the best candidate as a GitHub issue. This skill does **not** implement anything. Its deliverables are exactly three: 1. A verdict on two axes: **compatibility confidence** (can a native path be 100% Spark-compatible?) and **native upside** (how much throughput or allocation does the native path save versus codegen dispatch?). 2. A dated `Native candidate` line in the expression's page under `docs/source/contributor-guide/expression-audits/`, so a future run can see that this expression was already assessed and why it was or was not taken. 3. One filed GitHub issue for the recommended (or deferred-with-a-named-blocker) candidate. `$ARGUMENTS` may name a single expression to assess. If it is empty, run in survey mode (Step 1) and pick the best candidate from the whole pool. ## Background reading Read these before scoring anything. Do not score from memory: the routing model and the cost of the dispatcher are both easy to misremember, and every judgment in this skill depends on them. - `docs/source/contributor-guide/roadmap.md`, section "Native Coverage for Codegen-Dispatched Expressions". This is the project-level statement of why this work matters and it is the section the filed issue should reference. - `docs/source/user-guide/latest/compatibility/index.md`, section "Native and codegen-dispatch implementations". Defines the two paths and which one is the default. - `docs/source/user-guide/latest/expressions.md`, the Implementation legend and tables. Useful for a candidate's SQL name, category, and status, but see Step 1: this table is not the pool. - `docs/source/contributor-guide/optimizing_expressions.md`, the "Proven techniques" table. If no technique in that table applies to the candidate, the native upside claim is weak. - `spark/src/main/scala/org/apache/comet/serde/CometExpressionSerde.scala` for the `CometCodegenDispatch` / `CodegenDispatchFallback` / `NativeOptInAvailable` contracts. - `spark/src/main/scala/org/apache/comet/codegen/CometBatchKernelCodegen.scala` for what the dispatcher can and cannot compile (`isSupportedDataType`, `canHandle`). ## What the dispatcher actually costs Getting this right is the whole point of the skill. Overstating the dispatcher's cost produces issues that waste a contributor's week on a 2% win. What codegen dispatch **does** cost: - One JNI round trip per batch, out of native execution and back. - Per-row evaluation of Spark's generated Java. No vectorization, no SIMD, no dictionary awareness, no bulk buffer operations. - Per-row JVM heap allocation for every non-primitive intermediate and result: `UTF8String`, `Decimal`/`BigDecimal`, `GenericArrayData`, `ArrayBasedMapData`, boxed values, plus whatever Spark's own `doGenCode` allocates (formatters, `StringBuilder`, temporary arrays). - That allocation lands on the JVM heap, outside Comet's native memory pool, so it is invisible to Comet's memory accounting and it feeds GC pressure that shows up as task-level jitter rather than as an expression cost. - Per-row reads of nested Arrow data go through `CometArrayData` / `CometMapData` / `CometSpecializedGettersDispatch` wrappers, which allocate per row per nesting level. What codegen dispatch **does not** cost, and therefore is not available as an argument for going native: - It does **not** convert the whole batch to rows and back. The kernel reads Arrow vectors directly. - String reads are zero-copy (`UTF8String.fromAddress`). - The Janino kernel is compiled once per `(expression, schema)` pair, not per batch. - A `NullIntolerant` root short-circuits null rows without evaluating the body. - It is byte-exact with Spark by construction, so it needs no compatibility work at all. A native replacement starts with a compatibility debt that the dispatcher does not have. The corollary: a native implementation is worth proposing when the per-row JVM work is **allocation-heavy or vectorizable**, not merely because "native is faster than JVM". ## Step 1: Choose the candidate If `$ARGUMENTS` names an expression, skip to Step 2 but still run the exclusion checks below against it. ### Build the pool The **serde definitions are the source of truth**, not the docs table. Start there: ```bash # Sub-pool A: expressions whose only path is the dispatcher, plus those with a native opt-in. grep -rhoE "CometCodegenDispatch\[[A-Za-z0-9]+\]" spark/src/main/scala/org/apache/comet/serde/*.scala | sed 's/.*\[//;s/\]//' | sort -u # Which of those also have a native path (rendered "Hybrid" in the docs)? grep -rn "NativeOptInAvailable\|CodegenDispatchFallback" spark/src/main/scala/org/apache/comet/serde/*.scala ``` Then cross-reference `docs/source/user-guide/latest/expressions.md` for each candidate's SQL name, category, and Implementation label. Do **not** build the pool from that table alone: its Implementation column is derived from a function-registry lookup, so a dispatcher-routed serde whose SQL name does not resolve is rendered `—` and disappears from a table-driven grep. `mask` is the worked example: `CometMask extends CometCodegenDispatch[Mask]`, but its row reads `—`. At the time of writing the counts are 67 classes extending `CometCodegenDispatch` against 58 rows labelled `Codegen dispatch`, so a table-driven pool silently loses candidates. Two sub-pools, both in scope, with different proposed approaches: - **A. Dispatch-only** (a plain `CometCodegenDispatch`, no native path). The proposal is a new native kernel or an upstream wiring. - **B. Hybrid** (`NativeOptInAvailable` / `CodegenDispatchFallback`: a native path exists but is `Incompatible`, so **the dispatcher is still what runs by default**). These are in scope because the user-visible default is identical to sub-pool A, and so is the win from fixing it. The proposal is different: close the compatibility gap, or add a runtime guard that routes only the genuinely incompatible inputs back through the dispatcher, so the native path can become the default. The mechanics belong to `audit-comet-expression`; this skill's job is to establish that the win is worth someone's time. Roughly 42 expressions sit here, so skipping them would drop the largest block of candidates on a technicality. Out of scope: rows marked `—` or `🔜 Planned` where Comet has **no** path at all and the whole operator falls back to Spark. Those are `implement-comet-expression` territory, because the question is "implement it at all" rather than "is native better than dispatch". Their upside is genuinely larger than anything in this pool, so if you are looking for the biggest win available, say so and point the user at that skill rather than silently ranking within this pool as if it were the whole space. ### Exclude 1. **Families the project has ruled out.** Read the "Not currently planned" section of `docs/source/user-guide/latest/expressions.md` and drop anything it covers. Proposing one of these needs a project-direction argument that this skill does not have. 2. **Already-assessed expressions.** This is the check that makes the skill cumulative: ```bash grep -rn "Native candidate (assessed" docs/source/contributor-guide/expression-audits/ ``` An expression with a `Disqualified` line is off the pool unless the **named blocker** in that line has demonstrably changed (an upstream kernel landed, DataFusion gained the function, the Spark semantics were simplified in a new version). If it has changed, say so explicitly in the new assessment and add a new dated line. Never delete or rewrite an old line. 3. **Expressions with an existing issue.** Search open and closed: ```bash gh issue list --repo apache/datafusion-comet --search "<function> native in:title,body" --state all --limit 10 ``` If a candidate match comes back, open it (`gh issue view <N> --repo apache/datafusion-comet`) and confirm it really covers a native implementation of this expression. If it does, that expression is done: pick another rather than filing a duplicate. ### Rank what is left Rank the remaining pool by a cheap first-pass read of the two axes below, then assess the top candidate properly. Prefer expressions that appear in TPC-H or TPC-DS, or in common ETL shapes (string cleanup, date bucketing, JSON field extraction), over expressions that are rare in real queries. To check workload presence: ```bash grep -rli "<function>(" spark/src/test/resources/tpcds/ spark/src/test/resources/tpch/ 2>/dev/null ``` State the pool size, the exclusions, and why the chosen candidate topped the ranking. A ranking you do not show is a ranking the reader cannot challenge. ## Step 2: Establish how the expression runs today Find the serde and read it: ```bash grep -rn "class\|object" spark/src/main/scala/org/apache/comet/serde/*.scala | grep -i "<Expr>" ``` Record: - Which trait it uses, which decides the sub-pool and therefore the shape of the proposal. A plain `CometCodegenDispatch[T]` means dispatch is the only path (sub-pool A). `NativeOptInAvailable` or `CodegenDispatchFallback` means a native path already exists but is off by default (sub-pool B), so the candidate is not "write a kernel" but "make the existing kernel the default": establish which inputs actually diverge, and whether the divergence is detectable well enough to guard. Read the existing `getIncompatibleReasons()` and treat each listed reason as a claim to verify, not a given. - Whether the dispatcher can actually compile it, per `CometBatchKernelCodegen.canHandle` and `isSupportedDataType`. If some input or output type is outside that surface, those cases fall back to **Spark**, not to the dispatcher. Whole-operator Spark fallback is a much larger cost than a JNI round trip, and it raises the upside score materially. Say which cases these are. - Whether the expression is disabled or gated by any config (`spark.comet.expression.<Name>.enabled`, `allowIncompatible`, `spark.comet.exec.scalaUDF.codegen.enabled`). ## Step 3: Score compatibility confidence The question is narrow: **can a native Rust implementation match Spark bit-for-bit, for every input, on every supported Spark version?** "Close enough" is not a passing answer, because the dispatcher it would replace is exact. ### Read the Spark implementation across versions Derive the version list from the build rather than hardcoding it, so the assessment does not go stale as Comet drops and adds Spark support: ```bash grep -oE "<spark.version>[0-9.]+</spark.version>" pom.xml | sed -E 's/<[^>]+>//g' | sort -u ``` Read `eval` / `nullSafeEval` / `doGenCode`, `inputTypes`, `dataType`, ANSI branches, and any config the expression reads, for each of those versions. The class body is usually short enough to fetch directly instead of cloning: ```bash curl -sSfL "https://raw.githubusercontent.com/apache/spark/v<version>/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/<file>.scala" ``` Diff the class body between adjacent versions rather than eyeballing it. Note behavior that differs: each divergence is a shim the native path would need, and "identical across all supported versions" is itself a finding worth stating, because it means no shim. ### Check upstream first An existing upstream implementation is the single strongest positive signal, both for compatibility and for effort: ```bash grep -rn "fn name" ~/.cargo/registry/src/*/datafusion-spark-*/src/function/ 2>/dev/null | grep -i "<function>" grep -rn "fn name" ~/.cargo/registry/src/*/datafusion-functions-*/src/ 2>/dev/null | grep -i "<function>" ``` If `datafusion-spark` has it, read the implementation and diff its semantics against Spark's before crediting it. Upstream functions in that crate aim for Spark compatibility but are not all complete. If it matches, the proposal becomes a wiring job (`wire-datafusion-function`), which is a much cheaper issue to file and a much easier one to pick up. ### Hazard checklist Each hazard that applies is a strike against compatibility confidence. Check every one and report the result, including the ones that do not apply, so the reader can see the checklist was run rather than skimmed. | Hazard | Why it threatens 100% compatibility | | ----------------------------------------------------------------------------------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------- | | JVM formatting APIs (`java.text.DecimalFormat`, `String.format`, `SimpleDateFormat`, `DateTimeFormatter`) as in `format_number`, `format_string`, `to_char` | Locale, pattern, and rounding behavior would have to be reimplemented, not merely ported | | `java.util.regex` features (backreferences, lookaround, possessive quantifiers, named groups) | The Rust `regex` crate deliberately omits these, so some patterns cannot be supported at all | | Collation (`StringTypeWithCollation`, `CollationSupport.X.exec`) on Spark 4.0+ | ICU-backed collation is not propagated natively today ([#4496](https://github.com/apache/datafusion-comet/issues/4496)) | | `BigDecimal` / `MathContext` precision and rounding, precision-scale promotion, overflow-to-null | Rounding mode and overflow points are easy to get subtly wrong and produce silent wrong results | | Timezone and calendar (JDK tzdb versus `chrono-tz`, DST edges, hybrid Julian/Gregorian rebase, `spark.sql.legacy.*` flags) | Divergences appear only on specific dates and zones, so tests rarely catch them | | ANSI error class and message parity | An error with the wrong class or message is an observable difference; see the ANSI section of the compatibility guide | | Float and double text conversion (`Double.toString` shortest round-trip), `StrictMath` versus `f64` intrinsics | Last-digit differences on string output, platform-dependent differences on transcendentals | | Raw byte semantics of `UTF8String` (invalid UTF-8) ([#4764](https://github.com/apache/datafusion-comet/issues/4764)) | Arrow requires valid UTF-8, so byte-preserving behavior cannot be reproduced | | Lambda bodies or arbitrary child expression trees (`transform`, `aggregate`, `zip_with`) | The native path would need to evaluate an arbitrary Catalyst tree, not a fixed kernel | | Dependence on JVM session state beyond a scalar config (SQLConf lookups, user classes, reflection) | State that cannot be serialized into the plan cannot cross into native code | | Nondeterminism or ordering-sensitivity (hash iteration order, unstable sorts) | Output order must match Spark's, which is often an implementation detail rather than a spec | | Behavior that differs across the Spark versions pinned in `pom.xml` | Each divergence is a shim; several divergences means the native path carries version branches forever | ### Rate it - **High**: no hazard applies, or every hazard that applies is fully specified and mechanically reproducible (for example integer arithmetic with a documented overflow rule). A reviewer reading the Spark source could enumerate every case. - **Medium**: hazards apply but are bounded and detectable **from the expression or the batch at runtime**, so the incompatible cases can be routed back through the dispatcher via `CodegenDispatchFallback` while the common case runs natively. Name the guard and say how it is detected. "We will document the difference" is not a guard. - **Low**: matching Spark requires reimplementing a JVM library (ICU collation, `DecimalFormat`), or the incompatible cases cannot be detected before evaluating, or the only honest native path is `Incompatible` by default. A `Low` rating is a hard disqualifier regardless of upside. The project's direction is the opposite of shipping incompatible-by-default native paths (see [#4506](https://github.com/apache/datafusion-comet/issues/4506) and the discussion in [#4654](https://github.com/apache/datafusion-comet/issues/4654)). Verify both issues are still open before citing them. ## Step 4: Score native upside Read Spark's `doGenCode` (and `nullSafeEval`) for the expression and answer, concretely: **what does the JVM allocate or recompute per row, and can a Rust kernel avoid it?** Positive signals, strongest first: 1. **Per-row heap allocation in the generated code.** Count the allocation sites: `new UTF8String`, `UTF8String.fromString`, `.getBytes()`, `Decimal.apply`, `new GenericArrayData`, `new StringBuilder`, boxing. Multiply by batch size (8192) to state the allocation the native path removes. This is the most defensible upside argument, and it is the one that maps directly to the "reduction in memory allocation" question. 2. **Work that is constant across the batch but repeated per row.** A regex compiled per row, a formatter constructed per row, a literal argument re-parsed per row. Native kernels hoist this (see the technique table). 3. **A technique from `optimizing_expressions.md` that plainly applies.** Vectorized `unary`/`binary` kernels, zero-copy buffer reuse, `Cow` borrow, an ASCII byte-offset fast path, a lookup table, direct offset-buffer reads. Name the technique and the shape it helps. 4. **Nested or complex input/output types.** Per-row reads of `ListVector` / `StructVector` / `MapVector` through the JVM wrappers allocate per row per level. Native code operates on the child arrays and offsets directly. 5. **Specific inputs of an in-pool expression that fall back to Spark entirely today** (found in Step 2: an input or output type outside `isSupportedDataType`, so the dispatcher cannot compile the kernel and the whole operator falls back). Recovering those inputs dwarfs removing a JNI