rich7420 commented on code in PR #5835:
URL: https://github.com/apache/datafusion-comet/pull/5835#discussion_r4362755510
##########
spark/src/main/scala/org/apache/comet/serde/hash.scala:
##########
@@ -72,13 +72,16 @@ object CometMurmur3Hash extends
CometExpressionSerde[Murmur3Hash] {
}
}
-object CometSha2 extends CometExpressionSerde[Sha2] {
+// Non-foldable numBits uses Spark codegen inside the Comet pipeline; foldable
values stay native.
+object CometSha2 extends CometExpressionSerde[Sha2] with
CodegenDispatchFallback {
+
+ override def hasConditionalNativeDefault: Boolean = true
private val nonFoldableNumBitsReason =
"The `numBits` argument must be a foldable literal value"
override def getUnsupportedReasons(): Seq[String] =
- HashUtils.unsupportedReasons :+ nonFoldableNumBitsReason
+ Seq(nonFoldableNumBitsReason)
override def getSupportLevel(expr: Sha2): SupportLevel = {
if (!expr.right.foldable) {
Review Comment:
Could we drop the foldability check instead? `SparkSha2` already handles a
`numBits` column row by row, so this could stay fully native.
##########
spark/src/test/scala/org/apache/comet/CometHashExpressionSuite.scala:
##########
@@ -113,6 +118,76 @@ class CometHashExpressionSuite extends CometTestBase with
AdaptiveSparkPlanHelpe
}
}
+ test("hash - TimeType falls back to Spark") {
+ assume(isSpark41Plus, "TimeType requires Spark 4.1+")
+ withSQLConf("spark.sql.timeType.enabled" -> "true") {
+ withTable("t") {
+ sql("CREATE TABLE t(c STRING) USING parquet")
+ sql("INSERT INTO t VALUES ('12:34:56'), ('00:00:00'), (null)")
+ checkSparkAnswerAndFallbackReasons(
+ "SELECT hash(to_time(c)) FROM t",
+ Set("`TimeType` is not supported"))
+ checkSparkAnswerAndFallbackReasons(
+ "SELECT xxhash64(to_time(c)) FROM t",
+ Set("`TimeType` is not supported"))
+ }
+ }
+ }
+
+ test("sha2 - non-foldable numBits routes through the codegen dispatcher") {
+ withTable("t") {
+ sql("CREATE TABLE t(payload STRING, num_bits INT) USING parquet")
Review Comment:
nit: Could we also test a BINARY payload column with these `num_bits` values?
--
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]
