You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@datafusion.apache.org by gi...@apache.org on 2026/10/12 15:43:08 UTC
(datafusion-comet) branch gh-readonly-queue/main/pr-6776-695ca3ab7e6edefffffd6e073069561e66086c50 created (now 38f394e270)
This is an automated email from the ASF dual-hosted git repository.
github-merge-queue[bot] pushed a change to branch gh-readonly-queue/main/pr-6776-695ca3ab7e6edefffffd6e073069561e66086c50
in repository https://gitbox.apache.org/repos/asf/datafusion-comet.git
at 38f394e270 docs: prefer behavior-named parameters over Spark versions (#6776)
This branch includes the following new commits:
new 38f394e270 docs: prefer behavior-named parameters over Spark versions (#6776)
The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datafusion.apache.org
For additional commands, e-mail: commits-help@datafusion.apache.org
(datafusion-comet) 01/01: docs: prefer behavior-named parameters over Spark versions (#6776)
Posted by gi...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-6776-695ca3ab7e6edefffffd6e073069561e66086c50
in repository https://gitbox.apache.org/repos/asf/datafusion-comet.git
commit 38f394e2707814020cbf057510e25e85f07e47a8
Author: Andy Grove <ag...@apache.org>
AuthorDate: Mon Oct 12 15:00:48 2026 +0000
docs: prefer behavior-named parameters over Spark versions (#6776)
* 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.
* docs: address review on behavior-named parameters
Reconcile the review skill's shim paragraph with the new rule: the
version check may live in a shim or a named predicate in Scala, and
protobuf and native code see only the behavior. Extend the rule to
native function names, point to RegrSparkVersions for fixes that land
in patch releases, ask comments to cite the Spark JIRA, state the fork
benefit as it is today, and replace the removed isSpark33Plus helper.
The two existing version-named names are tracked in #6891.
---
.ai/skills/implement-comet-expression/SKILL.md | 3 ++-
.ai/skills/review-comet-expression-pr/SKILL.md | 4 +++-
.ai/skills/review-comet-pr/SKILL.md | 17 +++++++++++++----
.ai/skills/wire-datafusion-function/SKILL.md | 2 ++
.../source/contributor-guide/adding_a_new_expression.md | 16 +++++++++++++++-
5 files changed, 35 insertions(+), 7 deletions(-)
diff --git a/.ai/skills/implement-comet-expression/SKILL.md b/.ai/skills/implement-comet-expression/SKILL.md
index 529293f378..1ee9eff091 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/<category>/$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/<category>/$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 17f8e2f04c..c1a8077396 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 version check, or with a
+ protobuf field, native argument, or native function 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 e93361b69f..ea1f57a9d1 100644
--- a/.ai/skills/review-comet-pr/SKILL.md
+++ b/.ai/skills/review-comet-pr/SKILL.md
@@ -171,10 +171,19 @@ Report the comparison in the review even when nothing changed, so the reviewer k
### Spark version coverage
-Comet supports several Spark versions. Version-specific behavior belongs in the shims under
-`spark/src/main/spark-{3.4,3.5,3.x,4.0,4.1,4.2}/org/apache/comet/shims/`, not in branches on a
-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.
+Comet supports several Spark versions. The check on the Spark version belongs in Scala, either in a
+shim under `spark/src/main/spark-*/` or in one named predicate such as `isSpark42Plus` or
+`RegrSparkVersions.slopeFiltersVarByPairNulls`. It does not belong in an ad hoc comparison on the
+version string in shared code, or 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.
+
+Protobuf fields, native function arguments, and native function names only ever see the behavior,
+such as `wrap_second_millisecond_overflow`, never the version, such as `spark_420_plus` (#6740). A
+fork that backports a Spark fix then changes the one Scala check that sets the flag and leaves the
+protobuf and Rust alone. Flag a version-named field, argument, or function, and comments that say
+"Spark 4.2" for behavior later releases inherit or that leave out the Spark JIRA.
+`Cast.is_spark4_plus` and `get_json_object_spark34` predate the rule and #6891 tracks renaming them,
+so flag them only when a PR adds behavior behind them.
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
diff --git a/.ai/skills/wire-datafusion-function/SKILL.md b/.ai/skills/wire-datafusion-function/SKILL.md
index 881d24291b..4688f076c1 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, or pick a function name, that describes the behavior rather than 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.<name>.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 a246ea70bb..20bcae06e6 100644
--- a/docs/source/contributor-guide/adding_a_new_expression.md
+++ b/docs/source/contributor-guide/adding_a_new_expression.md
@@ -582,7 +582,21 @@ Spark.
If the expression you're adding has different behavior across different Spark versions, you'll need to account for that in your implementation. There are two tools at your disposal to help with this:
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.
+2. Variables that correspond to the Spark version, such as `isSpark42Plus`, which can be used to conditionally execute code based on the Spark version.
+
+#### Name the behavior, not the Spark version
+
+When native code has to match a behavior that differs between Spark versions, decide which behavior applies in Scala and pass the result to native code under a name that describes the behavior, not the Spark version that introduced it. This covers protobuf fields, native function arguments, and native function names. For example, `TruncTimestamp` carries a `wrap_second_millisecond_overflow` flag, which the serde sets from `!isSpark42Plus`. It is not called `spark_420_plus`. The `filter_v [...]
+
+Set the flag in one place on the Scala side, in a shim or in the serde, so that a fork overrides only that. A helper such as `isSpark42Plus` is enough when the behavior changed in a minor release. When Spark changed it in patch releases, compare the full version in a named predicate, because a check on the minor version is wrong for every earlier patch. `RegrSparkVersions.r2DegenerateCasesSwapped` in `serde/aggregates.scala` is the model. It resolves SPARK-55969, which shipped in 3.5.9, [...]
+
+Reasons to prefer behavior names:
+
+- Some deployments run a Spark fork that backports fixes from newer open-source releases. Such a fork changes the one Scala check that sets the flag and leaves the protobuf and native code alone. A `spark_420_plus` flag cannot describe a fork that has one 4.2 fix but not the others.
+- 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. Cite the Spark JIRA that changed the behavior, as the `TruncTimestamp` serde does with SPARK-56663, because forks track backports by JIRA rather than by release. 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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datafusion.apache.org
For additional commands, e-mail: commits-help@datafusion.apache.org