mbutrovich commented on code in PR #6447: URL: https://github.com/apache/datafusion-comet/pull/6447#discussion_r4197855028
########## docs/source/contributor-guide/floating_point.md: ########## @@ -0,0 +1,192 @@ +<!-- +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. +--> + +# Floating-Point Semantics + +This page describes how Comet matches Spark's handling of `-0.0` and NaN in `FLOAT` and `DOUBLE` +values. It is aimed at contributors working on an expression or operator that compares, orders, +hashes, deduplicates or groups floating-point values, including values nested in arrays, structs +and maps. For user-facing differences from Spark, see +[Floating-point Number Comparison](../user-guide/latest/compatibility/floating-point.md). The work +to apply these rules systematically is tracked in +[#6385](https://github.com/apache/datafusion-comet/issues/6385). + +The short version: Spark has no single rule for floating-point equality. Each function inherits one +from the Java API that its implementation calls. Arrow and DataFusion follow IEEE 754 total order, +which matches none of them. A native implementation must follow the rule of the Spark function it +replaces, using the shared helpers in `native/spark-expr/src/float_semantics/`, and its tests must +include a NaN with the sign bit set. + +## Spark's rules + +| Rule | `-0.0` and `0.0` | NaN | Used by | +| ----------------------------------------------------------- | ------------------------- | ---------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| SQL ordering (`SQLOrderingUtil.compareDoubles`, `genEqual`) | Equal | All NaNs are equal, and NaN sorts above all values | Comparisons, `IN`, sorting and ranking, `min`/`max`, `greatest`/`least`, `sort_array`, `array_min`/`array_max`, `array_contains`, `array_position`, `array_remove`, and comparisons of arrays and structs | +| `NormalizeNaNAndZero`, inserted by Spark's optimizer | `-0.0` becomes `0.0` | Every NaN becomes the canonical NaN | Grouping keys, join keys and window partition keys | +| `Murmur3Hash` and `XxHash64` | Same hash | Hashed through `doubleToLongBits`, which canonicalizes NaN | `hash`, `xxhash64` and hash partitioning | +| `java.lang.Double.equals`, for boxed values | Distinct | All NaNs are equal | Hash sets and maps of boxed values | +| `java.lang.Double.compare` | `-0.0` sorts before `0.0` | As in SQL ordering | An ascending `sort_array` of elements that cannot be null, in Spark's generated code | + +Some functions changed rules in a Spark release, so a native path has to follow the Spark version +it runs against: + +- `collect_set` keys its buffer by `Double.equals` before Spark 4.2. Spark 4.2 normalizes NaN and Review Comment: > Please describe Scala collection equality separately and classify pre-4.2 `collect_set` under that rule. `OpenHashSet`, which line 138 of this page lists under `Double.equals`, also depends on the Spark version. Before [SPARK-45599](https://issues.apache.org/jira/browse/SPARK-45599), `addWithoutResize` matched a key with `_data(pos) == k` ([3.4.3](https://github.com/apache/spark/blob/1eb558c3a6fbdd59e5a305bc3ab12ce748f6511f/core/src/main/scala/org/apache/spark/util/collection/OpenHashSet.scala#L149)), while `DoubleHasher` hashes the `doubleToLongBits` of the value ([3.4.3](https://github.com/apache/spark/blob/1eb558c3a6fbdd59e5a305bc3ab12ce748f6511f/core/src/main/scala/org/apache/spark/util/collection/OpenHashSet.scala#L312-L317)). So two NaNs never match, and `-0.0` and `0.0` start from different slots and match only when probing from one reaches the other. That holds for every 3.4 release, including 3.4.4, and for 3.5.0 and 3.5.1. The [fix](https://github.com/apache/spark/commit/b4118e0dbb505033d6a6a63b4c22c4e4bcf3e8d8) switched to `equals` in 3.5.2 and 4.0.0. When this rule gets r ewritten, could the page also say that `OpenHashSet` follows `Double.equals` only from 3.5.2 and 4.0.0? -- 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]
