andygrove commented on code in PR #5034: URL: https://github.com/apache/datafusion-comet/pull/5034#discussion_r3693103684
########## spark/src/test/resources/sql-tests/expressions/misc/uuid_with_seed.sql: ########## @@ -0,0 +1,79 @@ +-- 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. + +-- The one argument uuid(seed) form only exists in Spark 4.0+. With a fixed seed the output is +-- deterministic, so Comet must reproduce Spark's output exactly. Comet drives the same Commons +-- Math3 MersenneTwister as org.apache.spark.sql.catalyst.util.RandomUUIDGenerator, combining the +-- seed with the partition index, so these queries assert bit-for-bit equality with Spark in the +-- default query mode. + +-- MinSparkVersion: 4.0 + +statement +CREATE TABLE test_uuid_seed(id int) USING parquet + +statement +INSERT INTO test_uuid_seed VALUES (1), (2), (3), (4), (5) + +-- Multi-partition table used by the DISTRIBUTE BY query below to exercise partitionIndex != 0. +statement +CREATE TABLE test_uuid_parts(id int) USING parquet + +statement +INSERT INTO test_uuid_parts SELECT id FROM range(0, 32) + +-- fixed seed, single row -- also exercises the no-scan (OneRowRelation) planning path +query +SELECT uuid(0) + +-- pin the no-scan path with a deterministic assertion on top of the raw value above +query +SELECT length(uuid(0)) + +-- fixed seed, multiple rows: the generator advances per row within the partition +query +SELECT uuid(42) FROM test_uuid_seed + +-- Forces a hash exchange so uuid runs post-shuffle across several partitions. Locks the +-- `seed + partitionIndex` offset used by RandomUUIDGenerator; a single-partition test would +-- silently agree with Spark even if Comet ignored partitionIndex. +query +SELECT uuid(42) FROM test_uuid_parts DISTRIBUTE BY id Review Comment: You are right about the plan shape, and the query was covering nothing. Fixed in 2f6a5839e with your rewrite: ```sql SELECT uuid(42) FROM (SELECT id FROM test_uuid_parts DISTRIBUTE BY id) ``` On the companion partition-count query: I tried it and it cannot work in this suite, for a reason worth recording. `count(DISTINCT spark_partition_id())` is rejected outright — `[AGGREGATE_FUNCTION_WITH_NONDETERMINISTIC_EXPRESSION]` — and materialising it first (`count(DISTINCT pid) FROM (SELECT spark_partition_id() AS pid FROM ...)`) does run, but it still asserts nothing: `CometSqlFileTestSuite` compares Comet against **Spark**, so if the plan ever collapsed to one partition, Spark would collapse identically, both sides would return the same value, and the check would pass. There is no expected-literal mode to pin it against. So the assumption is only assertable from Scala, and that is where I put it: `CometUuidExpressionSuite` has a multi-partition test that does `assert(repartitioned.rdd.getNumPartitions > 1)` before comparing. The fixture now carries a comment explaining why the check lives there rather than inline. I also verified the coverage is real rather than assuming it this time. Mutating `UuidBuilder::build` to drop the offset: ```rust let seed = expr.seed; // was expr.seed.wrapping_add(planner.partition().into()) ``` fails both new Scala tests. Restored, and everything passes on spark-3.5 and spark-4.1. ########## spark/src/test/resources/sql-tests/expressions/misc/uuid.sql: ########## @@ -0,0 +1,43 @@ +-- 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. + +-- uuid() runs natively. The raw random value cannot be compared against Spark because Spark assigns +-- a fresh random seed on each planning pass, so these queries assert deterministic properties that +-- hold identically on both engines. The seeded uuid(seed) form (Spark 4.0+) asserts bit-for-bit +-- equality with Spark and lives in uuid_with_seed.sql. + +statement +CREATE TABLE test_uuid(id int) USING parquet + +statement +INSERT INTO test_uuid VALUES (1), (2), (3), (4), (5) + +-- canonical form is 36 characters +query +SELECT length(uuid()) FROM test_uuid + +-- matches the RFC 4122 version 4 layout (version nibble 4, variant nibble 8/9/a/b), lowercase hex. +-- This regex subsumes the length, alphabet, version, and variant checks. +query +SELECT uuid() RLIKE '^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$' FROM test_uuid + +-- Two unseeded uuid() nodes get distinct fresh random seeds during analysis (Uuid is stateful, +-- freshCopyIfContainsStatefulExpression rewrites aliases). Every row must therefore compare +-- unequal -- if Comet ever hoisted a single instance, this would flip to true. +-- spark_answer_only: value is random, so we only cross-check that Spark and Comet agree. +query spark_answer_only +SELECT uuid() = uuid() FROM test_uuid Review Comment: Agreed — dropped in 2f6a5839e, now default `query` mode. You are right that the comparison is deterministic even though the values are not: the two nodes get distinct fresh seeds, so it is `false` on every row on both engines. It passes on 3.5 and 4.1 in default mode, so the nativeness assertion the guard depends on is now actually being made. -- 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]
