comphead commented on code in PR #5348: URL: https://github.com/apache/datafusion-comet/pull/5348#discussion_r3778877834
########## .claude/skills/suggest-native-expression/SKILL.md: ########## @@ -0,0 +1,497 @@ +--- +name: suggest-native-expression +description: Use when picking the next Spark expression to implement natively in Comet, or when asked whether a specific codegen-dispatched expression is worth a native Rust implementation. Scores the candidate on how likely a native path can be 100% Spark-compatible and how much throughput or allocation it would save versus the JVM codegen dispatcher, records the verdict in the per-expression audit log so disqualified candidates are not re-litigated, and files a GitHub issue for the recommendation. +argument-hint: [expression-name] +--- + +<!-- +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. The + `Codegen dispatch` rows are the candidate 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 + +```bash +grep -n "| Codegen dispatch |" docs/source/user-guide/latest/expressions.md +``` + +Rows marked `Hybrid` are **out of scope**: a native path already exists there and the work is to +close its compatibility gap, which is `audit-comet-expression` territory. Rows marked `—` or +`🔜 Planned` are out of scope too: there is nothing to compare against, so the question is +"implement it at all", which is `implement-comet-expression`. + +### 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. A plain `CometCodegenDispatch[T]` means dispatch is the only path. + `CodegenDispatchFallback` means a native path exists for some inputs and dispatch catches the + rest, which narrows the candidate to the uncovered inputs. +- 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 + +Reuse the clone recipe from `audit-comet-expression` (Spark 3.4.3, 3.5.8, 4.0.1, 4.1.1) and read Review Comment: not sure if we need hardcode versions? should it be revised when support dropped/added? -- 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]
