andygrove commented on PR #5035: URL: https://github.com/apache/datafusion-comet/pull/5035#issuecomment-5080939030
Thanks for the thorough review @comphead. I merged latest `main` and pushed the additional coverage. Golden values were captured from real Spark 4.1.1 `ExpressionImplUtils.randStr`. **Rust tests (`randstr.rs`)** - `test_randstr_matches_spark_golden`: bit-for-bit fixtures for positive (42), zero (0), and negative (-1) seeds, asserted in-process independent of the SQL comparison. The negative-seed fixture also pins the `Int -> Long` sign extension. - `test_randstr_partition_index_seed`: exercises the `base_seed + partition_index` arithmetic by constructing `RandStrExpr::new(len, base + p)` and comparing against the seed-42 golden output. - `test_randstr_zero_length`: broadened across a wider seed set since the seed is irrelevant at length 0. - `test_randstr_large_length`: capacity-math smoke test, plus I switched the pre-size hint to `saturating_mul` so a huge length literal cannot overflow it. **SQL tests (`randstr.sql`)** - Int vs Long seed: `randstr(8, -1) = randstr(8, -1L)` runs natively and returns true. Note I used a `-1L` literal rather than `cast(-1 as bigint)`: the suite disables `ConstantFolding`, so the cast stays a non-literal and randstr falls back. - randstr feeding a filter: computed in a native projection, then filtered on the projected column. **A few I handled differently, happy to revisit:** - `parquet.enable.dictionary` ConfigMatrix: skipped. randstr ignores its input columns entirely, so parquet encoding cannot change the output, and the SQL-test guide asks not to add `ConfigMatrix` speculatively. - Multi-partition split-boundary SQL test: the partition-index seeding is now covered directly by the Rust test above. Comparing a per-partition-seeded nondeterministic expression across the Comet vs Spark scans is fragile (it relies on both engines producing identical file partitioning), so I kept the assertion at the unit level. - randstr in `WHERE`/`HAVING`: a nondeterministic predicate in a `Filter` is not accelerated (falls back to Spark), so a direct `WHERE randstr(...)` would fail the native assertion. The projection-fed filter above keeps randstr native while still testing the filtering context. Spark also restricts nondeterministic expressions in grouping/having. -- 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]
