docs: prefer behavior-named parameters over Spark versions

[andygrove]

Which issue does this PR close?

N/A. Follows the review discussion on #6740.

Rationale for this change

In #6740 the question came up of why the new protobuf flag is named wrap_second_millisecond_overflow rather than spark_420_plus. The reasoning is worth writing down so contributors and reviewers apply it consistently:

• A Spark fork that backports a fix changes the one Scala check that sets the flag, and leaves the protobuf and native code alone.
• Spark sometimes changes behavior in patch releases, so a version number is a poor proxy for behavior.
• Native code stays free of Spark version logic.

What changes are included in this PR?

• adding_a_new_expression.md: new "Name the behavior, not the Spark version" subsection under "API Differences Between Spark Versions". Protobuf fields, native function arguments, and native function names describe the behavior. The flag is set in one place in Scala, with RegrSparkVersions.r2DegenerateCasesSwapped as the model when Spark changed the behavior in patch releases. Comments cite the Spark JIRA and say "Spark 4.2 and later" when later releases inherit the behavior. The list above it now names isSpark42Plus instead of the removed isSpark33Plus.
• review-comet-pr: the "Spark version coverage" paragraph now puts the version check in a shim or in one named predicate in Scala, never in native Rust, and reviewers flag version-named fields, arguments, functions, and comments. Cast.is_spark4_plus and get_json_object_spark34 predate the rule and are tracked in Rename Cast.is_spark4_plus and get_json_object_spark34 to describe the behavior they select #6891.
• review-comet-expression-pr: the common-issues list covers version-named protobuf fields, native arguments, and native functions.
• implement-comet-expression and wire-datafusion-function: point authors to the new guide section.

How are these changes tested?

Documentation only. Checked with prettier.

[viirya]

Thanks for writing this down. Keeping proto fields and native code free of Spark version logic is a good rule, and pointing all four skills at one section of the guide keeps them from drifting. I left a few inline comments. The main one is that the new rule and the paragraph right above it in review-comet-pr currently disagree about where the version check may live, and #6740 itself would be flagged by the older paragraph. The other is that Cast.is_spark4_plus already breaks the new rule, so reviewers will start flagging it as soon as this lands.

[viirya]

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.

[andygrove]

Agreed, the two paragraphs contradicted each other. In 9bff1cf 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.

[viirya]

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.

[andygrove]

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.

[viirya]

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?

[andygrove]

Updated to isSpark42Plus in 9bff1cf.

[viirya]

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?

[andygrove]

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.

[viirya]

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.

[andygrove]

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.

[viirya]

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.

[andygrove]

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.

[sunchao]

Reviewed all five changed files at 40362298479575aa2eb5251d8c4e2f9a814cb682 against base 7d294535e87367d819c4e2a23422cd1158ac31b8, using their merge base a32529c4d84f5157be7a23aa0742ecdc15090eb7. There are no stacked prerequisite commits. The PR is not a draft.

Used review-comet-pr and review-comet-expression-pr for the expression guidance. Read the existing review, all six inline comments and threads, and the discussion snapshot. No substantiated unresolved P1/P2 blocker was identified.

Summary

• Prior state and problem: The guide described shims and version predicates without explaining why parameters crossing into native code should describe behavior rather than Spark releases.
• Design approach: Add that rationale to the expression guide and reinforce it in the two author skills and two review skills.
• Correctness: Checked Spark’s DateTimeUtils.truncTimestamp in 3.4.3, 3.5.9, 4.0.4, 4.1.3 and 4.2.0. Earlier versions use unchecked subtraction for SECOND/MILLISECOND truncation; 4.2.0 uses Math.subtractExact. The documented wrap_second_millisecond_overflow example and !isSpark42Plus polarity match #6740’s implementation in the supplied base. That implementation landed after this documentation branch diverged.
• Compatibility analysis: This PR changes no runtime code, protobuf schema, configuration or supported Spark version. Comparison with the changed files on branch-1.1 confirms this PR adds contributor policy without introducing a runtime compatibility change.
• Key design decisions: Resolve version differences in Scala and expose behavior through the native interface. Comments should describe inherited behavior as applying to “Spark 4.2 and later.”
• Implementation sketch: Add one guide subsection, an implementation checklist step, a wiring reminder and corresponding review checks. Both author-skill references resolve to the new section.
• Performance: The Markdown changes introduce no execution overhead, allocations or memory-use changes. No runtime performance improvement is claimed.
• Design: Extending the existing guide and workflows is a straightforward way to establish this convention. The existing discussion already covers the wording tension around serde versus shim placement.
• Abstraction & complexity: No implementation abstraction is introduced. Behavior-named parameters make the native contract understandable without requiring callers and kernels to share release-number assumptions.
• Behavioral changes worth calling out: Contributor and reviewer expectations become stricter about version-named parameters and comments. Query execution remains unchanged by this PR.
• Suggested improvements: No new change request meets the P1/P2 reporting bar. Existing comments are not duplicated.

Exact-head CI: seven checks succeeded and sixteen were skipped. Preflight, including Markdown formatting, and Required Checks passed. Linux/macOS builds, Spark SQL suites, Iceberg suites and site deployment were skipped. No check runs failed or remained pending.

Validation: local git diff --check and section-reference checks passed. Spark semantics were checked against source. No local JVM/native tests or Sphinx build were run; this review covers documentation-only changes. The checkout remains unchanged.

No introduced P1/P2 issues found within this review.