Skip to content

test: share collation coverage across Spark 4.x - #6495

Open
sunchao wants to merge 1 commit into
apache:mainfrom
sunchao:fix/shared-collation-suite
Open

sunchao wants to merge 1 commit into
apache:mainfrom
sunchao:fix/shared-collation-suite

Conversation

@sunchao

@sunchao sunchao commented Oct 1, 2026

Copy link
Copy Markdown
Member

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 sunchao added the run-all-spark-profiles Run the Comet test suites against every Spark profile on this pull request, ahead of the merge queue label Oct 1, 2026
@github-actions github-actions Bot added enhancement New feature or request test Testing related labels Oct 1, 2026

@sunchao sunchao left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
sunchao requested a review from andygrove October 1, 2026 15:50

@sunchao sunchao left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
sunchao requested review from comphead and viirya October 2, 2026 16:28

@sunchao sunchao left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request run-all-spark-profiles Run the Comet test suites against every Spark profile on this pull request, ahead of the merge queue test Testing related

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Move CometCollationSuite to spark-4.x so every 4.x profile runs it

1 participant