[sunchao]

Which issue does this PR close?

Closes #6233. Follow-up to #5302.

Rationale for this change

CometCollationSuite has separate Spark 4.0 and 4.1 copies and is absent from Spark 4.2. This leaves 4.2 without its collation fallback and datetime coverage, while the duplicated suites can drift.

What changes are included in this PR?

Move the common tests and both sort-merge join tests to spark-4.x, and remove the duplicate 4.1 suite. Keep the three hash-join tests in a Spark 4.0-only CometHashJoinCollationSuite, registered in both CI workflows. Spark 4.1+ normalizes collated hash-join keys to binary keys before the converter sees them, so those guard tests remain specific to 4.0.

The existing test bodies and assertions are preserved. Spark 4.0 retains all 28 tests (25 shared + 3 hash-join tests); Spark 4.1 gains the two sort-merge join tests; Spark 4.2 gains all 25 shared tests. Production behavior is unchanged.

How are these changes tested?

Local checks at 260ff8a5a, using JDK 21 and a debug native library built from the current base:

• make core: passed.
• Spark 4.0.4: all 28 tests passed (shared suite plus CometHashJoinCollationSuite).
• Spark 4.1.3: all 25 shared tests passed.
• Spark 4.2.0: all 25 shared tests passed.
• All three profile runs completed the full Maven reactor successfully, including Spotless and Scalastyle. No tests were ignored or canceled.
• python3 dev/ci/check-suites.py and git diff --check: passed.

The Spark profile runs used separate checkouts to avoid mixing compiled classes. Example commands, run from the repository root:

./mvnw -Pspark-4.0 test -Dtest=none \
  -Dsuites=org.apache.spark.sql.CometCollationSuite,org.apache.spark.sql.CometHashJoinCollationSuite
./mvnw -Pspark-4.1 test -Dtest=none -Dsuites=org.apache.spark.sql.CometCollationSuite
./mvnw -Pspark-4.2 test -Dtest=none -Dsuites=org.apache.spark.sql.CometCollationSuite

CI results are pending; run-all-spark-profiles requests coverage beyond the default 4.1 profile.

[sunchao]

Summary

• Prior state and problem: Collation coverage was duplicated across Spark 4.0 and 4.1 and absent from 4.2.
• Design approach: Move common coverage into spark-4.x and retain the three version-specific hash-join tests in CometHashJoinCollationSuite under spark-4.0.
• Correctness / compatibility analysis: Verified that all original test bodies are preserved byte-for-byte. Checked Spark 4.0.4, 4.1.3 and 4.2.0 sources. Spark 4.1+ hash-join constructors inject CollationKey, producing binary keys and justifying the version-specific split. The shared sort-merge and datetime checks remain applicable.
• Key design decisions: Existing Maven source sets handle version selection without introducing another abstraction. The new suite is registered in both Linux and macOS workflows.
• Implementation sketch: Rename the 4.0 suite into the shared source set, extract its hash-join tests, delete the duplicate 4.1 suite and update CI registrations.
• Behavioral changes worth calling out: Spark 4.0 retains 28 tests, 4.1 gains two sort-merge checks and 4.2 gains all 25 shared tests. Production code and runtime overhead are unchanged.
• Suggested improvements: None meeting the requested severity threshold. No introduced P1/P2 issues found within this review.

Reviewed the entire five-file diff from ec66afe56b1b9eb5ecc695169feb26495917eedd to 260ff8a5a8a0cc1a64d4de142d0eb69b8eb3037a. The PR is not a draft. Existing reviews, issue comments, inline comments and review threads were empty. Routed skills: review-comet-pr and review-comet-expression-pr.

Exact-head CI: 41 checks passed and 15 were skipped, with no failures or pending checks. CI logs confirm all 25 shared tests passed on each Spark 4.x profile, plus all three hash-join tests on 4.0. None of these cases were ignored or canceled.

Local validation passed: test-body preservation comparison, python3 dev/ci/check-suites.py and git diff --check. I did not rebuild JVM/native artifacts or rerun Scala suites locally. Runtime validation relies on exact-head Linux CI. macOS, upstream Spark SQL and Iceberg suites were skipped.

[sunchao]

Summary

• Prior state and problem: Collation tests were duplicated across Spark 4.0 and 4.1, with no corresponding suite for 4.2.
• Design approach: Share CometCollationSuite through spark-4.x and retain three hash-join tests in the 4.0-specific CometHashJoinCollationSuite.
• Correctness / compatibility analysis: All existing test bodies are preserved byte-for-byte. Spark 4.0.4, 4.1.3 and 4.2.0 sources support the split: 4.1+ hash joins normalize collated keys through CollationKey, producing binary keys. The shared sort-merge and datetime assertions remain applicable.
• Key design decisions: Existing Maven source sets select the appropriate suites without adding another abstraction. Both Linux and macOS workflows register the extracted suite.
• Implementation sketch: Move the 4.0 suite into the shared source set, extract its hash-join tests and remove the duplicate 4.1 suite.
• Behavioral changes worth calling out: Spark 4.0 retains all 28 tests, 4.1 gains two sort-merge tests and 4.2 gains all 25 shared tests. Production behavior and runtime overhead are unchanged.
• Suggested improvements: No introduced P1/P2 issues found within this review.

Reviewed the entire five-file diff from ec66afe56b1b9eb5ecc695169feb26495917eedd to 260ff8a5a8a0cc1a64d4de142d0eb69b8eb3037a. The PR is not a draft. Read the existing review and checked issue comments, inline comments and review threads. No substantiated unresolved P1/P2 concerns remain. Routed skills: review-comet-pr and review-comet-expression-pr.

Exact-head CI: 41 checks passed and 15 were skipped, with no failures or pending checks. CI logs confirm 25 shared tests passed on each Spark 4.x profile, plus three hash-join tests on 4.0. None of these cases were ignored or canceled.

Local validation passed: test-body preservation comparison, python3 dev/ci/check-suites.py and git diff --check. I did not rebuild JVM/native artifacts or rerun Scala suites locally. Runtime validation relies on CI for the requested head and base. macOS, upstream Spark SQL and Iceberg suites were skipped.

[sunchao]

Summary

• Prior state and problem: Collation tests were duplicated across Spark 4.0 and 4.1 and absent from 4.2.
• Design approach: Share CometCollationSuite through spark-4.x, keeping three hash-join tests in the 4.0-specific CometHashJoinCollationSuite.
• Correctness / compatibility analysis: Verified preservation of every original test body. Spark 4.0.4, 4.1.3 and 4.2.0 sources support the split: 4.1+ hash-join constructors normalize collated keys through CollationKey, producing binary keys. The shared sort-merge and datetime assertions remain applicable.
• Key design decisions: Existing Maven source sets select the suites without introducing another abstraction. Both Linux and macOS workflows register the extracted suite.
• Implementation sketch: Move the common suite, extract the hash-join tests and remove the duplicate 4.1 suite.
• Behavioral changes worth calling out: Spark 4.0 retains 28 tests, 4.1 gains two sort-merge tests and 4.2 gains all 25 shared tests. Production behavior and runtime overhead are unchanged.
• Suggested improvements: No introduced P1/P2 issues found within this review.

Reviewed the entire five-file diff from ec66afe56b1b9eb5ecc695169feb26495917eedd to 260ff8a5a8a0cc1a64d4de142d0eb69b8eb3037a. The PR is not a draft. Read both existing reviews and checked the empty issue-comment, inline-comment and review-thread collections. No substantiated unresolved P1/P2 concerns remain. Routed skills: review-comet-pr and review-comet-expression-pr.

Exact-head CI: 41 checks succeeded and 15 were skipped, with no failures or pending checks. CI logs confirm the requested head/base combination and all 25 shared tests passing on each Spark 4.x profile, plus three hash-join tests on 4.0. None of these cases were ignored or canceled.

Local validation passed: test-body preservation comparison, python3 dev/ci/check-suites.py and git diff --check. I did not rebuild JVM/native artifacts or rerun Scala suites locally. Runtime validation relies on CI. macOS, upstream Spark SQL and Iceberg suites were skipped.

[andygrove]

Thanks @sunchao. #4864 has since moved the 4.1 copy to the new spark-4.1+ test source set, so 4.2 already runs those 20 tests, and this now conflicts. After rebasing, the file to delete is spark-4.1+/org/apache/spark/sql/CometCollationSuite.scala, or 4.1 and 4.2 would compile two CometCollationSuite classes. The dedup and the five extra 4.1+/4.2 tests (including both sort-merge join cases) are still worth having. Could you rebase and update the test counts in the description? Happy to approve once CI is green on the new head.

[sunchao]

Summary

• Prior state and problem: At the specified base, collation coverage was duplicated across Spark 4.0 and 4.1 and absent from 4.2.
• Design approach: Share CometCollationSuite through spark-4.x, retaining three hash-join converter tests in the 4.0-specific CometHashJoinCollationSuite.
• Correctness: Verified that every original test body is preserved, including parameterized datetime cases. Spark sources confirm that 4.1+ hash-join constructors inject CollationKey, producing binary keys and explaining the version-specific split. The direct sort-merge converter assertions remain applicable.
• Compatibility analysis: Checked Spark 4.0.4, 4.1.3 and 4.2.0 sources. The tested datetime implementations retain the relevant semantics. Maven selects the shared suite once for each 4.x profile and the extracted suite only for 4.0. Spark 3.x source selection is unaffected.
• Key design decisions: Source-set placement expresses the version boundary without runtime version checks. Both Linux and macOS workflows register the extracted suite.
• Implementation sketch: Move the 4.0 suite into the shared source set, extract its three hash-join tests, delete the duplicate 4.1 suite and update CI registrations.
• Performance: Production code is unchanged, so this introduces no query-runtime overhead. Additional CI execution corresponds to expanded coverage. No performance issue meeting the P1/P2 bar was identified.
• Design: The split follows the actual Spark behavior difference and removes a duplicated suite. Existing Maven source sets provide a straightforward solution.
• Abstraction & complexity: No new framework or inheritance layer is introduced. The small suite-local helpers keep converter assertions self-contained without adding unnecessary abstraction.
• Behavioral changes worth calling out: Relative to the specified base, Spark 4.0 retains 28 tests, 4.1 gains two sort-merge tests and 4.2 gains all 25 shared tests. Production behavior is unchanged.
• Suggested improvements: No additional improvement meets the reporting threshold. No introduced P1/P2 issues found within this review.

Reviewed the entire five-file diff from ec66afe56b1b9eb5ecc695169feb26495917eedd to 260ff8a5a8a0cc1a64d4de142d0eb69b8eb3037a. The PR is not a draft. Routed skills: review-comet-pr and review-comet-expression-pr.

Read all four existing reviews and checked the empty issue-comment, inline-comment and review-thread collections. The existing rebase request following #4864 concerns newer main. The requested tree contains no duplicate suite definition, so no unresolved P1/P2 defect was substantiated for this head/base. Any rebased result needs separate validation.

Exact-head CI: 41 checks succeeded and 15 were skipped, with no failed or incomplete checks. Linux CI logs confirm the requested head/base combination and all 25 shared tests passing on each Spark 4.x profile, plus three hash-join tests on 4.0. None of these collation cases were ignored or canceled.

Local validation passed: test-body preservation comparison, Maven source-set inspection, python3 dev/ci/check-suites.py and git diff --check. JVM/native artifacts were not rebuilt and Scala suites were not rerun locally. Runtime validation relies on exact-head CI. macOS, upstream Spark SQL and Iceberg suites were skipped.

[viirya]

The split looks right to me. In 4.1.3 and 4.2.0, BroadcastHashJoinExec.apply and ShuffledHashJoinExec.apply both route their keys through HashJoin.normalizeJoinKeys, which wraps collated keys in CollationKey, so the BHJ/SHJ converters never see a collated key there. SortMergeJoinExec doesn't normalize its keys, so keeping the sort-merge join guards in spark-4.x is correct.

To add to @andygrove's note about rebasing onto #4864: the file to delete is now spark-4.1+/org/apache/spark/sql/CometCollationSuite.scala. Nothing in that copy or in the 4.0 copy has changed since this branch's base apart from the rename, so the move should carry over cleanly. One correction on the counts, so the updated description comes out right: the spark-4.1+ suite already has 23 tests, because the make_timestamp/to_unix_timestamp loop registers 4. After the rebase, 4.1 and 4.2 go from 23 to 25, and the only additions are the two sort-merge join cases. 4.0 stays at 28 (25 shared plus 3 in CometHashJoinCollationSuite). Since the CI run in the description is for the old head, could you rerun it with run-all-spark-profiles after the rebase? That confirms 4.1 and 4.2 each compile a single CometCollationSuite.

Two small comment suggestions inline. Otherwise this looks good to me once it's rebased.

[viirya]

Could this block point to where the hash-join cases went? Someone reading the shared suite on 4.1+ will otherwise wonder why only sort-merge join is covered. Something like:

// Broadcast and shuffled hash join guards live in the Spark 4.0-only
// CometHashJoinCollationSuite: Spark 4.1+ normalizes hash join keys to binary
// before Comet's converters see them.

[viirya]

Could the comment name where the normalization happens? That would let a reader check the claim, and it tells whoever drops 4.0 support that this suite can go with it. Also, "its converters" reads as Spark's converters; I think Comet's is meant.

Suggested change

	// Spark 4.1 and later normalize hash join keys with CollationKey before constructing the
	// physical join, so its converters receive BinaryType keys instead of collated strings.
	// These tests exercise the Spark 4.0 path, where the converters see the original collation.
	// In Spark 4.1 and later, BroadcastHashJoinExec.apply and ShuffledHashJoinExec.apply route
	// their keys through HashJoin.normalizeJoinKeys, which wraps collated keys in CollationKey
	// (BinaryType), so Comet's converters never see a collated string key there. These tests
	// exercise the Spark 4.0 path, where the converters still see the original collation.