You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "sunchao (via GitHub)" <gi...@apache.org> on 2026/10/01 02:42:10 UTC
[PR] test: share collation coverage across Spark 4.x [datafusion-comet]
sunchao opened a new pull request, #6495:
URL: https://github.com/apache/datafusion-comet/pull/6495
## 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:
```sh
./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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] test: share collation coverage across Spark 4.x [datafusion-comet]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #6495:
URL: https://github.com/apache/datafusion-comet/pull/6495#discussion_r4316384351
##########
spark/src/test/spark-4.x/org/apache/spark/sql/CometCollationSuite.scala:
##########
@@ -81,9 +79,9 @@ class CometCollationSuite extends CometTestBase {
}
}
- // ---- Join collation guards (issue #4051) ----------------------------------------
+ // ---- Sort-merge join collation guards (issue #4051) ----------------------------------------
//
- // Comet's native join compares keys byte-by-byte, so 'a' and 'A' would not match
+ // Comet's native sort-merge join compares keys byte-by-byte, so 'a' and 'A' would not match
Review Comment:
Could this block point to where the hash-join cases went? Someone reading the shared suite on 4.1+ will otherwise wonder why only sort-merge join is covered. Something like:
```scala
// Broadcast and shuffled hash join guards live in the Spark 4.0-only
// CometHashJoinCollationSuite: Spark 4.1+ normalizes hash join keys to binary
// before Comet's converters see them.
```
##########
spark/src/test/spark-4.0/org/apache/spark/sql/CometHashJoinCollationSuite.scala:
##########
@@ -0,0 +1,147 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.spark.sql
+
+import org.apache.spark.sql.catalyst.expressions.AttributeReference
+import org.apache.spark.sql.catalyst.optimizer.{BuildLeft, BuildRight}
+import org.apache.spark.sql.catalyst.plans.Inner
+import org.apache.spark.sql.comet.{CometBroadcastHashJoinExec, CometHashJoinExec}
+import org.apache.spark.sql.execution.{LocalTableScanExec, SparkPlan}
+import org.apache.spark.sql.execution.joins.{BroadcastHashJoinExec, ShuffledHashJoinExec}
+import org.apache.spark.sql.types.StringType
+
+import org.apache.comet.{CometConf, CometExplainInfo}
+import org.apache.comet.serde.OperatorOuterClass
+
+// Spark 4.1 and later normalize hash join keys with CollationKey before constructing the
+// physical join, so its converters receive BinaryType keys instead of collated strings.
+// These tests exercise the Spark 4.0 path, where the converters see the original collation.
Review Comment:
Could the comment name where the normalization happens? That would let a reader check the claim, and it tells whoever drops 4.0 support that this suite can go with it. Also, "its converters" reads as Spark's converters; I think Comet's is meant.
```suggestion
// In Spark 4.1 and later, BroadcastHashJoinExec.apply and ShuffledHashJoinExec.apply route
// their keys through HashJoin.normalizeJoinKeys, which wraps collated keys in CollationKey
// (BinaryType), so Comet's converters never see a collated string key there. These tests
// exercise the Spark 4.0 path, where the converters still see the original collation.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org