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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
