andygrove commented on code in PR #6776: URL: https://github.com/apache/datafusion-comet/pull/6776#discussion_r4249169614
########## .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: Agreed, the two paragraphs contradicted each other. In 9bff1cf105 the first paragraph now says the version check belongs in Scala, either in a shim or in one named predicate such as `isSpark42Plus` or `RegrSparkVersions.slopeFiltersVarByPairNulls`, and not in an ad hoc comparison on the version string or in native Rust. The second paragraph is now only about what protobuf and native code see. That covers #6740's pattern and the way shared code already uses the `isSpark*Plus` helpers. ########## .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: I filed #6891 to rename both. On main `is_spark4_plus` only gates one behavior, leading padding before a time-only `T` (SPARK-46488), so it's a rename rather than a split. `get_json_object_spark34` has become misleading too, because since #6600 the 3.4 shim picks it from the Jackson version on the classpath rather than the Spark version. The rule now covers native function names, and the review skill points at #6891 and tells reviewers to flag those two names only when a PR adds behavior behind them. ########## 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: Updated to `isSpark42Plus` in 9bff1cf105. ########## 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: You're right, with #6740 a fork still patches shared Scala, just not the proto or the Rust. I did both. The bullet now says that a fork changes the one Scala check that sets the flag and leaves the protobuf and native code alone, and a new paragraph asks for the flag to be set in one place, in a shim or in the serde, so that a fork overrides only that. ########## 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: Added. The new paragraph says a helper such as `isSpark42Plus` is enough when the behavior changed in a minor release, and that a change in patch releases needs a named predicate on the full version, with `RegrSparkVersions.r2DegenerateCasesSwapped` as the model for SPARK-55969. `filter_var_by_pair_nulls` is now in the first paragraph as a second example of a behavior-named 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. +#### 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: Yes, the JIRA is a better anchor for forks than the release. The comments paragraph now asks for the Spark JIRA, with the `TruncTimestamp` serde's SPARK-56663 as the example, and the review skill flags a comment that leaves it out. -- 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]
