andygrove commented on code in PR #5081: URL: https://github.com/apache/datafusion-comet/pull/5081#discussion_r3684150969
########## spark/src/test/resources/sql-tests/expressions/math/arithmetic_ansi.sql: ########## @@ -147,6 +147,37 @@ SELECT c div d FROM ansi_div_zero query expect_error(BY_ZERO) SELECT c % d FROM ansi_div_zero +-- ============================================================================ +-- Float/Double remainder by zero (issue #5067) +-- Spark 4.1 throws REMAINDER_BY_ZERO for floating-point x % 0 as well. +-- ============================================================================ + +statement +CREATE TABLE ansi_float_div_zero(a float, b float, c double, d double) USING parquet + +statement +INSERT INTO ansi_float_div_zero VALUES (3.0, 0.0, 3.0, 0.0), (1.0, -0.0, 1.0, -0.0) + +-- float column % 0 should throw +query expect_error(BY_ZERO) +SELECT a % b FROM ansi_float_div_zero WHERE b = 0.0 + +-- double column % 0 should throw +query expect_error(BY_ZERO) +SELECT c % d FROM ansi_float_div_zero WHERE d = 0.0 + +-- literal double % 0.0 should throw +query expect_error(BY_ZERO) +SELECT CAST(1.0 AS DOUBLE) % CAST(0.0 AS DOUBLE) Review Comment: Agreed. Added non-foldable cases in 6c815fea, keeping a column operand so `ConstantFolding` can't evaluate them: ```sql SELECT a % CAST(0.0 AS FLOAT) FROM ansi_float_div_zero SELECT c % CAST(0.0 AS DOUBLE) FROM ansi_float_div_zero SELECT c % CAST(-0.0 AS DOUBLE) FROM ansi_float_div_zero SELECT CAST(1.0 AS DOUBLE) % d FROM ansi_float_div_zero SELECT c % CAST(2.0 AS DOUBLE) FROM ansi_float_div_zero ORDER BY 1 ``` The last one is a plain `query` rather than `expect_error`, so a non-zero literal divisor still has to produce correct results and not throw. I kept the three fully-literal cases, since they still assert Spark's own behavior, but added a comment above them noting they're folded before reaching Comet so nobody reads them as native coverage. Rather than assume these reach the native path, I checked: I temporarily disabled the new float branch in `spark_modulo` *and* removed the two pre-existing `a % b` / `c % d` column-vs-column cases, then reran the file. It still failed with `cometError.isDefined was false Expected Comet to throw an error matching 'BY_ZERO' but query succeeded`, so the new cases are doing the work on their own. Restored both and the file passes. One note for anyone reproducing locally: my Maven run resolved to the Spark 4.1 / Scala 2.13 profile, which needs JDK 17 — JDK 11 fails to compile `ShimCometSparkSessionExtensions.scala` with `Class java.lang.Record not found`. That profile is the one where `REMAINDER_BY_ZERO` actually applies, so it's the useful one to run here. -- 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]
