From 40362298479575aa2eb5251d8c4e2f9a814cb682 Mon Sep 17 00:00:00 2001 From: Andy Grove Date: Thu, 8 Oct 2026 08:49:07 -0600 Subject: [PATCH] docs: prefer behavior-named parameters over Spark versions When behavior varies between Spark versions, resolve the version in Scala and pass native code a parameter that describes the behavior. Add the guidance to the contributor guide and the expression skills. --- .ai/skills/implement-comet-expression/SKILL.md | 3 ++- .ai/skills/review-comet-expression-pr/SKILL.md | 4 +++- .ai/skills/review-comet-pr/SKILL.md | 6 ++++++ .ai/skills/wire-datafusion-function/SKILL.md | 2 ++ .../contributor-guide/adding_a_new_expression.md | 12 ++++++++++++ 5 files changed, 25 insertions(+), 2 deletions(-) diff --git a/.ai/skills/implement-comet-expression/SKILL.md b/.ai/skills/implement-comet-expression/SKILL.md index 529293f3784..1ee9eff091f 100644 --- a/.ai/skills/implement-comet-expression/SKILL.md +++ b/.ai/skills/implement-comet-expression/SKILL.md @@ -77,7 +77,8 @@ Follow `adding_a_new_expression.md`: 2. Register it in the matching map in `QueryPlanSerde.scala`. 3. If the function name collides with a DataFusion built-in that has a different signature, use `scalarFunctionExprToProtoWithReturnType` (see "When to set the return type explicitly"). 4. For a new scalar function, add a match case in `native/spark-expr/src/comet_scalar_funcs.rs::create_comet_physical_fun`. If step 2 found an upstream implementation, wire that in. Otherwise implement the function under `native/spark-expr/src/`. -5. Add at least one Comet SQL Test at `spark/src/test/resources/sql-tests/expressions//$ARGUMENTS.sql` exercising column references, literals, and `NULL`. +5. If Spark's behavior differs between versions, resolve the version in the serde or a shim and pass native code a parameter named for the behavior (`wrap_second_millisecond_overflow`), not the version (`spark_420_plus`). See "Name the behavior, not the Spark version" in `adding_a_new_expression.md`. +6. Add at least one Comet SQL Test at `spark/src/test/resources/sql-tests/expressions//$ARGUMENTS.sql` exercising column references, literals, and `NULL`. Build and smoke-test: diff --git a/.ai/skills/review-comet-expression-pr/SKILL.md b/.ai/skills/review-comet-expression-pr/SKILL.md index 17f8e2f04ce..f07df2760f9 100644 --- a/.ai/skills/review-comet-expression-pr/SKILL.md +++ b/.ai/skills/review-comet-expression-pr/SKILL.md @@ -266,7 +266,9 @@ reference in that doc is the only place they are documented. 3. **Wrong return type**, it must match Spark exactly 4. **Tests in the wrong framework**, Scala tests where a SQL file test would do 5. **Missing `getSupportLevel`**, divergences left undeclared rather than marked `Incompatible` -6. **Version-specific Spark behavior implemented once**, with no shim +6. **Version-specific Spark behavior implemented once**, with no shim, or a shim whose protobuf + field or native parameter is named for the Spark version (`spark_420_plus`) instead of the + behavior (`wrap_second_millisecond_overflow`) 7. **Name collides with a DataFusion built-in** and no explicit return type 8. **Timestamp result mislabelled**, with the session timezone or no timezone instead of `"UTC"`. It only shows once the result is compared or fed to another expression. diff --git a/.ai/skills/review-comet-pr/SKILL.md b/.ai/skills/review-comet-pr/SKILL.md index e93361b69f9..1a795fbd6fd 100644 --- a/.ai/skills/review-comet-pr/SKILL.md +++ b/.ai/skills/review-comet-pr/SKILL.md @@ -176,6 +176,12 @@ Comet supports several Spark versions. Version-specific behavior belongs in the version string in shared code, and not in native Rust. If the PR adds a shim for one 4.x version, check that the sibling 4.x source sets got it too. +Parameters, protobuf fields, and native function arguments that vary with the Spark version should +describe the behavior, such as `wrap_second_millisecond_overflow`, not the version, such as +`spark_420_plus`. Forks that backport fixes can then set the flag from their own shim, and the +native code carries no version logic (#6740). Flag a version-named parameter, and comments that say +"Spark 4.2" for behavior later releases inherit. + When Spark changed the behavior in a patch release, such as SPARK-55969 or SPARK-54918, a check on the minor version is wrong for every earlier patch. CI builds only the newest patch of each line, so it can't catch that (#6042, #5701). The pull request CI also runs only the default Spark profile, so diff --git a/.ai/skills/wire-datafusion-function/SKILL.md b/.ai/skills/wire-datafusion-function/SKILL.md index 881d24291bf..20c86b2f2d9 100644 --- a/.ai/skills/wire-datafusion-function/SKILL.md +++ b/.ai/skills/wire-datafusion-function/SKILL.md @@ -102,6 +102,8 @@ Helpers from `QueryPlanSerde`: - `optExprWithInfo(optExpr, expr, children*)` — wrap final result; propagates "why we couldn't convert" tags. - `withInfo(expr, "reason")` — tag a fallback when returning `None`. +If the Spark behavior you are matching varies by Spark version, resolve the version in Scala and pass the native side a parameter named for the behavior, not the version (see "Name the behavior, not the Spark version" in `adding_a_new_expression.md`). + `getSupportLevel` returning `Incompatible(Some("…"))` gates behind `spark.comet.expr..allowIncompatible=true`. `Unsupported(…)` always falls back. ### 4. Register the UDF (Pattern B only) diff --git a/docs/source/contributor-guide/adding_a_new_expression.md b/docs/source/contributor-guide/adding_a_new_expression.md index d9e7707bab4..e038e9ced8a 100644 --- a/docs/source/contributor-guide/adding_a_new_expression.md +++ b/docs/source/contributor-guide/adding_a_new_expression.md @@ -583,6 +583,18 @@ If the expression you're adding has different behavior across different Spark ve 1. Shims that exist in `spark/src/main/spark-$SPARK_VERSION/org/apache/comet/shims/CometExprShim.scala` for each Spark version. These shims are used to provide compatibility between different Spark versions. 2. Variables that correspond to the Spark version, such as `isSpark33Plus`, which can be used to conditionally execute code based on the Spark version. +#### Name the behavior, not the Spark version + +When a code path or a protobuf field depends on how Spark behaves, name it for the behavior and not for the Spark version that introduced it. Resolve the version in Scala, in the serde or a shim, and pass the result to native code as a parameter that describes the behavior. For example, `TruncTimestamp` carries a `wrap_second_millisecond_overflow` flag, which the serde sets from `!isSpark42Plus`. It is not called `spark_420_plus`. + +Reasons to prefer behavior names: + +- Some deployments run a Spark fork that backports fixes from newer open-source releases. They can set a behavior flag in their own shim, but cannot make a `spark_420_plus` flag mean something it does not. +- Spark occasionally changes behavior in a patch release or between minor releases, so a version number is a poor proxy for the behavior. +- The native code stays free of Spark version logic, and a reader of the Rust or proto definition can tell what the flag does without looking up a release. + +Write comments the same way. Say "Spark 4.2 and later" for a behavior that future releases inherit, rather than "Spark 4.2". If a later release changes the behavior again, add a new parameter or shim for that change. + ## Shimming to Support Different Spark Versions If the expression you're adding has different behavior across different Spark versions, you can use the shim system located in `spark/src/main/spark-$SPARK_VERSION/org/apache/comet/shims/CometExprShim.scala` for each Spark version.