Conversation
sunchao
left a comment
There was a problem hiding this comment.
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.xand retain the three version-specific hash-join tests inCometHashJoinCollationSuiteunderspark-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
left a comment
There was a problem hiding this comment.
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
CometCollationSuitethroughspark-4.xand retain three hash-join tests in the 4.0-specificCometHashJoinCollationSuite. - 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
left a comment
There was a problem hiding this comment.
Summary
- Prior state and problem: Collation tests were duplicated across Spark 4.0 and 4.1 and absent from 4.2.
- Design approach: Share
CometCollationSuitethroughspark-4.x, keeping three hash-join tests in the 4.0-specificCometHashJoinCollationSuite. - 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.
Which issue does this PR close?
Closes #6233. Follow-up to #5302.
Rationale for this change
CometCollationSuitehas 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-onlyCometHashJoinCollationSuite, 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.CometHashJoinCollationSuite).python3 dev/ci/check-suites.pyandgit diff --check: passed.The Spark profile runs used separate checkouts to avoid mixing compiled classes. Example commands, run from the repository root:
CI results are pending;
run-all-spark-profilesrequests coverage beyond the default 4.1 profile.