viirya commented on code in PR #6776:
URL: https://github.com/apache/datafusion-comet/pull/6776#discussion_r4226163963
##########
.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
Review Comment:
`Cast.is_spark4_plus` in `expr.proto` already breaks this rule, and its
comment says it gates more than one behavior ("such as the handling of leading
whitespace before T-prefixed time-only strings"). Once this lands, any PR that
touches `Cast` will get flagged for it. Could we file a tracking issue to split
it into behavior-named fields and link it here? Renaming the field keeps its
number, so it stays wire-compatible because the JVM and native sides ship
together.
Should the rule also cover native function names? `get_json_object_spark34`
is picked by the Spark 3.4 shim and has the same problem in a function name.
##########
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.
Review Comment:
The `TruncTimestamp` example resolves at minor-version granularity, so it
doesn't show this case. `RegrSparkVersions.r2DegenerateCasesSwapped` in
`serde/aggregates.scala` resolves a behavior flag down to the patch version for
SPARK-55969 (3.5.9, 4.0.3, 4.1.2, 4.2.0). Would it be worth pointing to it as
the model when a fix lands in patch releases? `filter_var_by_pair_nulls` is
also a good existing example of a behavior-named proto field.
##########
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.
Review Comment:
`isSpark33Plus` no longer exists. The helpers today are `isSpark35Plus`
through `isSpark42Plus`. Since the new section builds directly on this list,
could we update it to `isSpark42Plus` here?
##########
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.
Review Comment:
A fork can only set the flag "in their own shim" if the flag is resolved in
a shim. With #6740 it is resolved in the shared `datetime.scala`, so a fork
still patches shared Scala code. It just doesn't have to touch the proto or the
Rust. Could we state the benefit as what it is today, or recommend resolving
the flag in a shim or a single named predicate so that a fork overrides one
place?
##########
.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
Review Comment:
The paragraph just above says version-specific behavior belongs in the shims
and "not in branches on a version string in shared code". The guide added here
says to resolve the version "in the serde or a shim", and its example is
`builder.setWrapSecondMillisecondOverflow(!isSpark42Plus)` in the shared
`serde/datetime.scala`. A reviewer following both paragraphs would flag #6740's
own pattern.
Could we reconcile the two? One way is to say that the version check may
live in Scala, in a shim or as a named predicate in the serde like
`RegrSparkVersions.slopeFiltersVarByPairNulls`, and that what this rule
protects is that proto and native code only ever see the behavior.
##########
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.
Review Comment:
Should we also ask comments to cite the SPARK JIRA, as #6740 does with
SPARK-56663? Forks track backports by JIRA rather than by release, so it
supports the fork rationale above more directly than the version does.
--
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]
