You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "sam-1112 (via GitHub)" <gi...@apache.org> on 2026/08/19 09:19:55 UTC

[PR] test: fix vacuous signed-zero coverage in SQL file tests [datafusion-comet]

sam-1112 opened a new pull request, #5393:
URL: https://github.com/apache/datafusion-comet/pull/5393

   ## Which issue does this PR close?
   
   Closes #5271.
   
   ## Rationale for this change
   
   Bare `-0.0` literals in SQL file tests are parsed by Spark as `decimal(1,1)`, which has no signed zero. Coercion to `double` or `float` yields `+0.0`, so rows that appear to test signed-zero behaviour are actually inserting `+0.0` on both sides — the coverage is vacuous. `CAST(-0.0 AS DOUBLE)` and `CAST(-0.0 AS FLOAT)` have the same problem because the cast source is still a decimal literal.
   
   ## What changes are included in this PR?
   
   - Replaced all bare `-0.0` and `CAST(-0.0 AS DOUBLE/FLOAT)` literals across **32 SQL test files** with `double('-0.0')` / `float('-0.0')`, which parse through the string path and preserve the sign bit.
   - Added `query ignore(...)` annotations for two known Spark/Comet divergences that became visible once the literals were fixed:
     - `array_min`: Spark returns `+0.0` when `+0.0` and `-0.0` are both present; Comet returns `-0.0`.
     - `sort_array` descending: Spark keeps `-0.0` before `+0.0`; Comet reverses them.
   - Added a **Signed zero** note in `docs/source/contributor-guide/sql-file-tests.md` so future contributors know to use `double('-0.0')` / `float('-0.0')` when writing float edge-case fixtures.
   
   ## How are these changes tested?
   All modified SQL test files were run against both Spark 4.1 (default profile, JDK 17) and Spark 3.4 (`-Pspark-3.4`) and passed.
   ```sh
   ./mvnw test -Dtest=none -Dscalastyle.skip=true \
     -Dsuites="org.apache.comet.CometSqlFileTestSuite \
       array_compact array_distinct array_except array_insert array_intersect \
       array_max array_min array_repeat array_union arrays_overlap arrays_zip \
       shuffle shuffle_with_seed sort_array sort_array_strict_fp \
       cast_array_to_string cast_complex_types_to_string cast_double_to_string \
       boolean arithmetic_ansi atan2 bin csc rint sec tan string_replace \
       approx_count_distinct avg collect_list collect_set percentile"
   # repeated with -Pspark-3.4
   ```
   Spotless format check also passed on both Spark 4.1 (default profile) and Spark 3.4 (`-Pspark-3.4`).


-- 
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org

Re: [PR] test: fix vacuous signed-zero coverage in SQL file tests [datafusion-comet]

Posted by "sunchao (via GitHub)" <gi...@apache.org>.
sunchao commented on code in PR #5393:
URL: https://github.com/apache/datafusion-comet/pull/5393#discussion_r3812048072


##########
spark/src/test/resources/sql-tests/expressions/array/shuffle.sql:
##########
@@ -124,7 +124,7 @@ statement
 INSERT INTO test_shuffle_double VALUES
   (array(1.1, 2.2, 3.3, 4.4, 5.5)),
   (NULL),
-  (array(CAST('NaN' AS DOUBLE), CAST('Infinity' AS DOUBLE), CAST('-Infinity' AS DOUBLE), 0.0, -0.0))
+  (array(CAST('NaN' AS DOUBLE), CAST('Infinity' AS DOUBLE), CAST('-Infinity' AS DOUBLE), 0.0, double('-0.0')))

Review Comment:
   [P2] Keep the unseeded permutation check deterministic
   
   Could this mixed signed-zero case use a comparison that does not depend on the order of equal zeros? The query below compares `sort_array(shuffle(arr))` exactly, but Spark's nullable-array comparator treats `0.0` and `-0.0` as equal. Its stable sort therefore preserves their randomly shuffled order. The Spark and Comet runs resolve independent seeds, and the result comparator distinguishes the zero bits. With the new row, repeated Spark 4.1.1 executions produce both orders, while the baseline's two positive zeros were stable. This introduces a flaky SQL test. Sorting the string representations, for example `sort_array(transform(shuffle(arr), x -> cast(x AS string)))`, preserved both zero signs and produced one result across seeds 0 through 11 in a Spark-only probe. Could we use that kind of canonical multiset comparison 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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org

Re: [PR] test: fix vacuous signed-zero coverage in SQL file tests [datafusion-comet]

Posted by "sam-1112 (via GitHub)" <gi...@apache.org>.
sam-1112 commented on code in PR #5393:
URL: https://github.com/apache/datafusion-comet/pull/5393#discussion_r3813064693


##########
spark/src/test/resources/sql-tests/expressions/array/shuffle.sql:
##########
@@ -124,7 +124,7 @@ statement
 INSERT INTO test_shuffle_double VALUES
   (array(1.1, 2.2, 3.3, 4.4, 5.5)),
   (NULL),
-  (array(CAST('NaN' AS DOUBLE), CAST('Infinity' AS DOUBLE), CAST('-Infinity' AS DOUBLE), 0.0, -0.0))
+  (array(CAST('NaN' AS DOUBLE), CAST('Infinity' AS DOUBLE), CAST('-Infinity' AS DOUBLE), 0.0, double('-0.0')))

Review Comment:
   Thanks, that was a real flake. Spark's stable sort keeps the shuffled order of +0.0 and -0.0, while the SQL test comparator distinguishes the bits, so `sort_array(shuffle(arr))` is not unique once the fixture actually contains both signs.
   
   I switched the unseeded double projection to `sort_array(transform(shuffle(arr), x -> cast(x AS string)))` so the comparison is a canonical multiset that still preserves both zero signs. The seeded fixture is unchanged and still checks the exact permutation.
   
   I also noted the `sort_array` pitfall next to the signed-zero guidance in `sql-file-tests.md`.



-- 
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org

Re: [PR] test: fix vacuous signed-zero coverage in SQL file tests [datafusion-comet]

Posted by "sam-1112 (via GitHub)" <gi...@apache.org>.
sam-1112 commented on code in PR #5393:
URL: https://github.com/apache/datafusion-comet/pull/5393#discussion_r3819199522


##########
spark/src/test/resources/sql-tests/expressions/math/csc.sql:
##########
@@ -19,7 +19,7 @@ statement
 CREATE TABLE test_csc(d double) USING parquet
 
 statement
-INSERT INTO test_csc VALUES (0.0), (-0.0), (1.5707963267948966), (-1.5707963267948966), (3.141592653589793), (NULL), (cast('NaN' as double)), (cast('Infinity' as double)), (cast('-Infinity' as double))
+INSERT INTO test_csc VALUES (0.0), (double('-0.0')), (1.5707963267948966), (-1.5707963267948966), (3.141592653589793), (NULL), (cast('NaN' as double)), (cast('Infinity' as double)), (cast('-Infinity' as double))

Review Comment:
   Thanks — confirmed. `checkAnswerWithTolerance` only tests `isInfinity` and absolute difference, so the corrected `csc(-0.0)` / `tan(-0.0)` inputs still could not detect a sign flip. I kept `tolerance=1e-6` for ordinary trig values and added plain `query` assertions for the zero column and literal cases in `csc.sql` and `tan.sql`.
   `atan2.sql` had the same hole for the ±0 results (Spark canonicalizes both args with `+0.0`, so a leftover `-0.0` would also pass under tolerance). Those cases now use exact comparison too; the ±π cases stay on the tolerance queries. I also noted this in `sql-file-tests.md` next to `query tolerance=` and the signed-zero guidance.



-- 
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org

Re: [PR] test: fix vacuous signed-zero coverage in SQL file tests [datafusion-comet]

Posted by "sunchao (via GitHub)" <gi...@apache.org>.
sunchao commented on code in PR #5393:
URL: https://github.com/apache/datafusion-comet/pull/5393#discussion_r3814291350


##########
spark/src/test/resources/sql-tests/expressions/array/array_repeat.sql:
##########
@@ -39,8 +39,8 @@ INSERT INTO test_array_repeat VALUES
   (CAST(127 AS TINYINT), CAST(32767 AS SMALLINT), 2147483647, 9223372036854775807, CAST('Infinity' AS FLOAT), CAST('Infinity' AS DOUBLE), CAST(99999999999999999999999999999999999999 AS DECIMAL(38, 0)), true, 'a', X'01', DATE '1970-01-01', TIMESTAMP '1970-01-01 00:00:00', array(1, 2, 3), 3),
   (CAST(-128 AS TINYINT), CAST(-32768 AS SMALLINT), -2147483648, -9223372036854775808, CAST('-Infinity' AS FLOAT), CAST('-Infinity' AS DOUBLE), CAST(-99999999999999999999999999999999999999 AS DECIMAL(38, 0)), false, 'é', X'02FF', DATE '9999-12-31', TIMESTAMP '9999-12-31 23:59:59', array(), 2),
   (CAST(0 AS TINYINT), CAST(0 AS SMALLINT), 0, 0, CAST('NaN' AS FLOAT), CAST('NaN' AS DOUBLE), CAST(0 AS DECIMAL(38, 0)), false, '', X'', DATE '2024-02-29', TIMESTAMP '2024-02-29 12:34:56', array(NULL, 1, NULL), 1),
-  (NULL, NULL, NULL, NULL, CAST(0.0 AS FLOAT), CAST(-0.0 AS DOUBLE), NULL, NULL, '日本', X'00', NULL, NULL, NULL, 0),
-  (CAST(1 AS TINYINT), CAST(1 AS SMALLINT), 1, 1, CAST(-0.0 AS FLOAT), CAST(0.0 AS DOUBLE), CAST(1 AS DECIMAL(38, 0)), true, NULL, NULL, DATE '2000-01-01', TIMESTAMP '2000-01-01 00:00:00', array(7), -1),
+  (NULL, NULL, NULL, NULL, CAST(0.0 AS FLOAT), double('-0.0'), NULL, NULL, '日本', X'00', NULL, NULL, NULL, 0),
+  (CAST(1 AS TINYINT), CAST(1 AS SMALLINT), 1, 1, float('-0.0'), CAST(0.0 AS DOUBLE), CAST(1 AS DECIMAL(38, 0)), true, NULL, NULL, DATE '2000-01-01', TIMESTAMP '2000-01-01 00:00:00', array(7), -1),

Review Comment:
   [P2] Exercise signed zero with a positive repeat count
   
   Could we also run `array_repeat(float_v, 2)` and `array_repeat(double_v, 2)` against this table? Both corrected negative-zero cells currently have nonpositive counts, so their results are empty arrays. An implementation that loses the sign would still pass these tests, leaving this part of #5271 unverified. Please retain the existing zero- and negative-count cases.



-- 
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org

Re: [PR] test: fix vacuous signed-zero coverage in SQL file tests [datafusion-comet]

Posted by "sunchao (via GitHub)" <gi...@apache.org>.
sunchao commented on code in PR #5393:
URL: https://github.com/apache/datafusion-comet/pull/5393#discussion_r3818521549


##########
spark/src/test/resources/sql-tests/expressions/math/csc.sql:
##########
@@ -19,7 +19,7 @@ statement
 CREATE TABLE test_csc(d double) USING parquet
 
 statement
-INSERT INTO test_csc VALUES (0.0), (-0.0), (1.5707963267948966), (-1.5707963267948966), (3.141592653589793), (NULL), (cast('NaN' as double)), (cast('Infinity' as double)), (cast('-Infinity' as double))
+INSERT INTO test_csc VALUES (0.0), (double('-0.0')), (1.5707963267948966), (-1.5707963267948966), (3.141592653589793), (NULL), (cast('NaN' as double)), (cast('Infinity' as double)), (cast('-Infinity' as double))

Review Comment:
   [P2] Make the signed-zero trig checks sign-sensitive
   
   Could the zero cases here and in `tan.sql` also have plain `query` assertions? They still run only with `tolerance=1e-6`: `CometTestBase.checkAnswerWithTolerance` compares infinities only by `isInfinity` and finite values by absolute difference, so it accepts `+Infinity` for the expected `csc(-0.0) == -Infinity`, and `+0.0` for `tan(-0.0) == -0.0`. Replaying the exact Spark 4.1.1 fixture queries through that comparator with only these signs changed still passes all three assertions. These corrected inputs therefore remain unable to detect sign loss. Please retain tolerance for ordinary trig values and add exact signed-zero checks for the column and literal cases.



-- 
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org

Re: [PR] test: fix vacuous signed-zero coverage in SQL file tests [datafusion-comet]

Posted by "sunchao (via GitHub)" <gi...@apache.org>.
sunchao merged PR #5393:
URL: https://github.com/apache/datafusion-comet/pull/5393


-- 
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org