andygrove commented on PR #5034: URL: https://github.com/apache/datafusion-comet/pull/5034#issuecomment-5147052554
@mbutrovich all four addressed in 2f6a5839e, details inline. Two of them found real holes: **The multi-partition query was covering nothing.** Your reading of the plan shape is correct — `DISTRIBUTE BY` wraps the already-built Project, so uuid ran pre-shuffle on a single `FilePartition`. Moved the projection above the exchange. **The golden constants were the only guard on 3.4/3.5, and could have gone circular.** Added `CometUuidExpressionSuite`, which builds `Uuid(Some(seed))` directly through the existing version-shimmed `getColumnFromExpression`, so every profile now gets a bit-for-bit cross-engine assertion. Five seeds including negative/MIN/MAX, plus a multi-partition test that asserts `getNumPartitions > 1` before comparing. **I verified both are non-vacuous rather than asserting it.** Mutating `UuidBuilder::build` to drop `wrapping_add(planner.partition())` fails both new Scala tests; restored, everything passes on spark-3.5 and spark-4.1. **On the companion partition-count query — it cannot work here, and this is worth knowing for future fixtures.** `count(DISTINCT spark_partition_id())` is rejected outright with `[AGGREGATE_FUNCTION_WITH_NONDETERMINISTIC_EXPRESSION]`, and the materialised form runs but asserts nothing: this suite compares Comet against *Spark*, so a collapse to one partition would collapse both sides identically and still pass. There is no expected-literal mode. So the check is only meaningful from Scala, which is where it now lives; the fixture documents why. **`spark_answer_only`** dropped — you were right that the comparison is deterministic even though the values are not. **`typeof`:** I checked the mechanism, and your diagnosis holds — `TypeOf.foldable` is true so ConstantFolding normally removes the node, and this suite excludes ConstantFolding (`CometSqlFileTestSuite:91-92`), which is what exposes the unquoted-`catalogString` bug. My earlier "Spark 4.1 codegen" framing was wrong on both counts. Recorded as a harness constraint on the query, and I will fix the PR description. I have **not** filed the upstream Spark ticket — that is an outward-facing action on another project's tracker, so I would rather you confirm or take it; the repro is `SELECT typeof(<non-foldable>)` with ConstantFolding excluded, on 3.5, 4.0 or master. Verified: 5 Rust uuid unit tests pass; `uuid.sql`, `uuid_with_seed.sql` and both new Scala tests pass on spark-3.5 and spark-4.1. -- 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]
