rich7420 commented on code in PR #7114:
URL: https://github.com/apache/datafusion-comet/pull/7114#discussion_r4362871528
##########
docs/source/contributor-guide/floating_point.md:
##########
@@ -201,11 +201,13 @@ and only for a type that contains a `FLOAT` or `DOUBLE`.
`MaxSparkVersion`, and add the `run-all-spark-profiles` label to the pull
request. CI pins one
patch release per Spark line, so when a patch release changed the rule, also
test the routing in
Scala.
-- Add the expression to `CometFloatSemanticsSuite`, which crosses the edge
values with operator
- contexts and with every expression and aggregate that accepts floating-point
input. New
- expressions go in its `expressions` or `aggregates` list. If one does not
match Spark yet, add a
- `KnownGap` that links the issue. Known gaps are strict: a case that starts
matching Spark fails,
- so the fix also removes the entry.
+- Add the expression to the sweep in `float_semantics/`, which crosses the
edge values with
+ operator contexts and with every expression and aggregate that accepts
floating-point input.
+ A new expression goes in `expressions.sql`, or in `aggregates.sql` for an
aggregate, with one
+ query for `DOUBLE` and one for `FLOAT`. If one does not match Spark yet,
mark its queries
+ `ignore(<issue>)`. A SQL file test cannot expect a difference, so a gap that
should fail once it
+ is fixed goes in `CometFloatSemanticsSuite`, whose test for `collect_set`
before Spark 4.2 fails
+ when the answers start to match.
Review Comment:
Could a known mismatch go in a strict `CometFloatSemanticsSuite` test
instead of `ignore`, so the test fails once it's fixed?
```suggestion
query for `DOUBLE` and one for `FLOAT`. If one does not match Spark yet,
add a strict test in
`CometFloatSemanticsSuite` that links the issue and fails once the answers
match, like its
`collect_set` test before Spark 4.2.
```
##########
spark/src/test/resources/sql-tests/float_semantics/expressions.sql:
##########
@@ -0,0 +1,417 @@
+-- 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.
+
+-- Expressions with float input, over the edge values (one column) or every
ordered pair of
+-- them (two columns). Each follows one of Spark's float equality rules, which
the contributor
+-- guide lists (contributor-guide/floating_point.md). The sweep tracks #6385.
+--
+-- The e and p CTEs negate the flagged stored values to make the sign-bit NaN
at query time
+-- (see comparison.sql).
+--
+-- hash and xxhash64 of a float, an array and a struct are in
expressions/hash/hash.sql.
Review Comment:
Could we keep a `FLOAT` array hash case? `hash.sql` only hashes `array(-d)`,
so `hash(array(-f))` and `xxhash64(array(-f))` seem to lose coverage.
--
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]
