Skip to content

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

Description

@stantheman0128

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

  1. 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.
  2. 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.
  3. 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.

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or request

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions