What is the problem the feature request solves?
CometCollationSuite holds the collation fallback tests (#1947, #4051, #4646), but it only runs on some profiles. There are two copies, one under spark/src/test/spark-4.0 and one under spark/src/test/spark-4.1, and there is no spark/src/test/spark-4.2, so the 4.2 profile runs neither. None of its tests run on 4.2, including the DISTINCT, GROUP BY and ORDER BY fallbacks and the #4646 datetime checks.
The two copies have drifted in one place. The 4.1 copy has no #4051 join-key tests (the block at lines 81-247 of the 4.0 copy, plus its imports and the joinKeyCollationReason constant); everything else is the same. Spark 4.1 looks like the reason. BroadcastHashJoinExec and ShuffledHashJoinExec build their keys through HashJoin.normalizeJoinKeys there, which wraps a collated key in CollationKey, whose type is BinaryType. So the exec never receives a collated key and the converter has nothing to reject. SortMergeJoinExec does not normalize its keys, so the sort-merge join tests in that block should still hold on 4.1 and later.
Meanwhile new collation tests are going into separate suites under spark-4.x (CometCastCollatedStringSuite in #5302, CometSortCollationSuite in #6206). With CometCollationSuite in spark-4.x, they would have one place to go.
Describe the potential solution
- Move the shared part of the 4.0 copy to
spark/src/test/spark-4.x/org/apache/spark/sql/CometCollationSuite.scala and delete the 4.1 copy. Both would otherwise be test roots on the 4.1 profile (spark/pom.xml:726-728), and two classes would share one fully qualified name.
- Keep the broadcast and shuffled hash join tests where only 4.0 compiles and runs them, with a comment saying why they do not apply on 4.1 and later. Keep the sort-merge join tests on every 4.x profile if they pass there.
- Run the moved suite on 4.0, 4.1 and 4.2, and fix or document whatever the first 4.2 run turns up.
A related question for later: if Spark 4.1 already turns collated hash join keys into binary before Comet sees them, the #4051 guard may not be needed for those joins on 4.1 and later, and Comet could run them natively. That deserves its own look and is not part of this move.
Additional context
Came out of the #5302 review: #5302 (review)
Line numbers are from main at 646ff181.
What is the problem the feature request solves?
CometCollationSuiteholds the collation fallback tests (#1947, #4051, #4646), but it only runs on some profiles. There are two copies, one underspark/src/test/spark-4.0and one underspark/src/test/spark-4.1, and there is nospark/src/test/spark-4.2, so the 4.2 profile runs neither. None of its tests run on 4.2, including the DISTINCT, GROUP BY and ORDER BY fallbacks and the #4646 datetime checks.The two copies have drifted in one place. The 4.1 copy has no #4051 join-key tests (the block at lines 81-247 of the 4.0 copy, plus its imports and the
joinKeyCollationReasonconstant); everything else is the same. Spark 4.1 looks like the reason.BroadcastHashJoinExecandShuffledHashJoinExecbuild their keys throughHashJoin.normalizeJoinKeysthere, which wraps a collated key inCollationKey, whose type isBinaryType. So the exec never receives a collated key and the converter has nothing to reject.SortMergeJoinExecdoes not normalize its keys, so the sort-merge join tests in that block should still hold on 4.1 and later.Meanwhile new collation tests are going into separate suites under
spark-4.x(CometCastCollatedStringSuitein #5302,CometSortCollationSuitein #6206). WithCometCollationSuiteinspark-4.x, they would have one place to go.Describe the potential solution
spark/src/test/spark-4.x/org/apache/spark/sql/CometCollationSuite.scalaand delete the 4.1 copy. Both would otherwise be test roots on the 4.1 profile (spark/pom.xml:726-728), and two classes would share one fully qualified name.A related question for later: if Spark 4.1 already turns collated hash join keys into binary before Comet sees them, the #4051 guard may not be needed for those joins on 4.1 and later, and Comet could run them natively. That deserves its own look and is not part of this move.
Additional context
Came out of the #5302 review: #5302 (review)
Line numbers are from
mainat646ff181.