Repository navigation
fix: reject non-default collations in all sort keys - #6206
Conversation
|
Reopening after CI validation in my fork passed CI run. |
sunchao
left a comment
There was a problem hiding this comment.
Summary
- Prior state and problem: Multi-key sorts bypassed collation checks, allowing byte ordering to produce incorrect window row numbers and TopK results.
- Design approach: Reuse
hasNonDefaultStringCollationto check every sort key recursively before applying existing single-key restrictions. - Correctness / compatibility analysis: Spark sources confirm collation-aware string and nested-type ordering in 4.0.4, 4.1.3 and 4.2.0. The existing Spark 3.x shim leaves behavior unchanged for 3.4.3 and 3.5.9. Both
CometSortExecand TopK use the guarded method. - Key design decisions: Preserve native default-collation sorting and existing single-key restrictions. Reusing the helper keeps the change small. The additional traversal occurs during planning, with no new per-row work.
- Implementation sketch: Add the shared guard, seven regression tests, an adversarial WindowGroupLimit fixture value, and Linux/macOS suite registration.
- Behavioral changes worth calling out: Non-default collations in primary, secondary or nested sort keys now trigger Spark fallback with an explicit reason.
- Suggested improvements: None at the requested severity threshold. No introduced P1/P2 issues found within this review.
Reviewed all five changed files against base 801b099101b685d661e9332588f9de6fc9e194da at full head 3b556ae10794a87a5d4824df66432d5ed98acd06. The PR is not a draft. Existing discussion contained no unresolved review concerns. Routed skills: review-comet-pr and review-comet-expression-pr.
Exact-head CI: Upstream Comet CI and CodeQL report action_required. The fork CI run passed. Its tested merge tree exactly matches the reviewed head. Logs confirm all seven new tests and the updated SQL fixture passed, within 1,032 execution tests and 1,536 expression tests.
Validation limits: Local CI configuration checks and git diff --check passed. Local JVM/native suites were not run because this checkout lacks built native artifacts and Maven dependencies. The fork run skipped Spark's SQL suites, macOS and non-default Spark runtime profiles. The author's additional local results were not independently reproduced.
Which issue does this PR close?
Closes #6158.
Rationale for this change
Multi-column sort keys bypassed the collation check in
supportedSortType, allowing Comet to compare non-default collated strings by raw bytes. Arow_number()window overUTF8_LCASEand an integer tie-breaker consequently assigned different row numbers from Spark when a single-partition exchange bypassed the existing shuffle guards.What changes are included in this PR?
hasNonDefaultStringCollationhelper before applying the unchanged single-column sorting restrictions. Unsupported collations now fall back to Spark with an explicit reason, including in TopK plans.How are these changes tested?
The original window regression was run before the fix and failed with the exact incorrect row numbers from #6158. Its physical plan contained
CometWindowExecoverCometSortover aSinglePartitioncolumnar exchange. The same test passes after the fix and asserts Spark sort fallback.Verified locally:
CometSortCollationSuite, thewindow_group_limit_collationSQL fixture, and existingCometExecSuitetests matchingSort on.CometSortCollationSuite,CometCollationSuite, thewindow_group_limit_collationSQL fixture, and existingCometExecSuitetests matchingSort on.CometExecSuitematchingSort onandCometExpressionSuitematchingsort.git diff --checkpassed.