comphead commented on code in PR #5835: URL: https://github.com/apache/datafusion-comet/pull/5835#discussion_r4485784710
########## spark/src/test/resources/sql-tests/expressions/hash/sha2.sql: ########## @@ -0,0 +1,66 @@ +-- 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. + +-- ConfigMatrix: spark.comet.exec.scalaUDF.codegen.enabled=true,false Review Comment: `CometSha2` is not a `CodegenDispatchFallback`, so both runs of this matrix plan the same native `sha2`, and `expect_native(sha2)` already fails if it is ever routed through the dispatcher. Could the dispatcher axis go, here and in `sha2_ansi.sql`? If a second axis is wanted, `parquet.enable.dictionary=false,true` as in `hash.sql` would reach the cast of dictionary-encoded payloads to `BinaryType`. I have not run that variant. ########## spark/src/test/resources/sql-tests/expressions/hash/sha2.sql: ########## @@ -0,0 +1,66 @@ +-- 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. + +-- ConfigMatrix: spark.comet.exec.scalaUDF.codegen.enabled=true,false +-- ConstantFolding: enabled + +-- Keep payloads in Parquet columns so SHA-2 executes even when numBits is folded. +statement +CREATE TABLE test_sha2_string(payload STRING, num_bits INT) USING parquet + +statement +INSERT INTO test_sha2_string VALUES + ('hello', 0), ('hello', 224), ('hello', 256), ('hello', 384), ('hello', 512), + ('hello', 128), ('hello', -1), ('hello', NULL), ('', 256), ('', 512), (NULL, 256) + +-- Row-varying bit widths, including invalid widths and NULL, must stay native. +query expect_native(sha2) +SELECT sha2(payload, num_bits) FROM test_sha2_string + +query expect_native(sha2) +SELECT sha2(payload, num_bits + 0) FROM test_sha2_string + +query expect_native(sha2) +SELECT sha2('hello', num_bits) FROM test_sha2_string + +-- Literal widths and a foldable width expression exercise the existing native path. +query expect_native(sha2) +SELECT sha2(payload, 0), sha2(payload, 224), sha2(payload, 256), Review Comment: `hash.sql` already runs `sha2(col, 0/256/224/384/512/128/-1)` over string columns under `expect_native` (around line 83). Could this keep only what is new, the BINARY payload and the `128 + 128` width? ########## spark/src/test/scala/org/apache/spark/sql/benchmark/CometHashCodegenDispatchBenchmark.scala: ########## @@ -0,0 +1,234 @@ +/* + * 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.benchmark + +import java.nio.charset.StandardCharsets + +import org.apache.spark.benchmark.Benchmark +import org.apache.spark.sql.Row +import org.apache.spark.sql.catalyst.optimizer.ConstantFolding +import org.apache.spark.sql.internal.SQLConf + +import org.apache.comet.CometConf +import org.apache.comet.udf.codegen.CometScalaUDFCodegen + +/** + * Benchmark of native non-foldable `sha2(payload, numBits)` against Spark. + * + * Both Comet arms execute SHA-2 natively, with the codegen dispatcher enabled and disabled. The + * mixed projection also measures native expressions alongside `sha2`. + * + * Before timing anything, every case is run through all three arms and the rows are compared, so + * a timing cannot come from an arm that computed something else. + * + * To run this benchmark: + * {{{ + * SPARK_GENERATE_BENCHMARK_FILES=1 make benchmark-org.apache.spark.sql.benchmark.CometHashCodegenDispatchBenchmark + * }}} + * Results will be written to "spark/benchmarks/CometHashCodegenDispatchBenchmark-**results.txt". + */ +object CometHashCodegenDispatchBenchmark extends CometBenchmarkBase { + + /** Fewer rows than one Comet batch, so the query is a single batch of real work. */ + private val SmallRows = 1024 + + /** ~128 batches at the default `spark.comet.batchSize`. */ + private val LargeRows = 1024 * 1024 + + /** + * @param name + * Case name, as it appears in the results table. + * @param query + * The query to time. Every argument is a column so that the expression is evaluated per row. + */ + private case class DispatchCase(name: String, query: String) + + private def cases: Seq[DispatchCase] = Seq( + DispatchCase("sha2(payload, numBits)", "select sha2(c_str, c_bits) from parquetV1Table"), Review Comment: `CometHashExpressionBenchmark` already benchmarks `sha2_224` to `sha2_512`, and `runExpressionBenchmark` already disables constant folding and warns on a non-native plan. Could the column-valued width be one more `HashExprConfig` there, for example `sha2(c_str, element_at(array(0, 224, 256, 384, 512, 128), c_int % 6 + 1))`? The dispatch on and off arms here run the same native path (108 ms against 108 ms in the PR's table), and `CometCodegenDispatchBenchmark` already exists for the dispatcher. ########## spark/src/test/scala/org/apache/comet/CometHashExpressionSuite.scala: ########## @@ -113,6 +114,22 @@ class CometHashExpressionSuite extends CometTestBase with AdaptiveSparkPlanHelpe } } + test("hash - TimeType falls back to Spark") { Review Comment: This `TimeType` test for `hash` and `xxhash64`, the class comment and the `isSpark41Plus` import are left from the earlier dispatcher scope, and `TimeType` already falls back on `main`. They also make this file conflict with #7484 (`git merge-tree` reports a content conflict only here). Could they be dropped from this PR? ########## docs/source/contributor-guide/expression-audits/hash_funcs.md: ########## @@ -54,14 +54,14 @@ ## sha1 - Spark 3.4.3 (audited 2026-05-27): identical to 3.5.8. -- Spark 3.5.8 (audited 2026-05-27): baseline. `Sha1(child) extends UnaryExpression with NullIntolerant`; `inputTypes = Seq(BinaryType) -> StringType`. Comet routes via `CometSha1` to the native `sha1` UDF. +- Spark 3.5.8 (audited 2026-05-27): baseline. `Sha1(child) extends UnaryExpression with NullIntolerant`; `inputTypes = Seq(BinaryType) -> StringType`. The Analyzer casts accepted inputs to `BinaryType` before Comet serde, and `CometSha1` routes the resulting binary child to the native `sha1` UDF. Review Comment: The `sha1` note here, the `xxhash64` sentence on line 73 and the `sha` row note in `expressions.md` describe behavior this PR does not change (wide-decimal hashing is #6548). Could this keep only the `sha2` line? -- 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]
