andygrove commented on code in PR #6447: URL: https://github.com/apache/datafusion-comet/pull/6447#discussion_r4199398258
########## 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: You're right, thanks. `mutable.HashSet[Any]` compares boxed values with Scala's `==`, which merges the two zeros and never matches a NaN, the opposite of `Double.equals` on both counts. 6f81c8202 gives Scala `==` its own row in the rules table and moves pre-4.2 `collect_set` under it. I reproduced it on 3.4.3 and 4.1.3: `size(collect_set(d))` is 3 for a group holding `-0.0`, `0.0` and two NaNs, and 4 for three NaNs and two `1.0`s. ########## 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: Thanks, confirmed. 3.4.3 has `_data(pos) == k`, and the 3.5 backport of SPARK-45599 is in 3.5.2 but not 3.5.1. 6f81c8202 says that `OpenHashSet` and `OpenHashMap` follow `Double.equals` only from 3.5.2 and 4.0.0, and describes the older matching. That qualifies the `mode` bullet too, because `mode` counts values in an `OpenHashMap`: on 3.4.3, `mode` over three NaNs and two `1.0`s returns `1.0`, where 4.1.3 returns NaN. The new bullet also names the other users, `percentile` and the four array set functions, which handle NaN separately. ########## 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. + Review Comment: It holds. On 3.4.3 and 4.1.3 I wrote four dictionary-encoded files, two whose only zero is `-0.0` and two whose only zero is `0.0`. With either Parquet reader, `d = -0.0D`, `d <=> -0.0D` and `d IN (-0.0D, 5.0D)` returned only the `-0.0` rows, and turning off `parquet.filter.dictionary.enabled` or `spark.sql.parquet.filterPushdown` brought the `0.0` rows back. It isn't limited to equality either. `DictionaryFilter` orders the entries with `Double.compare`, so `d >= 0.0D` skipped a row group whose largest value is `-0.0`, and `d <= -0.0D` one whose smallest is `0.0`. Comet returned every row in each case. 6f81c8202 adds a paragraph with those examples to the compatibility guide, after the one on pruning. It says that Comet's rows are the ones Spark's comparison semantics call for, and that Spark returns them too with `spark.sql.parquet.filterPushdown=false`. The contributor guide's data filter bullet says the same, and that a pruning test writes out its expected rows. The `CometNativeReaderSuite` bloom test writes with dictionary encoding off, so there the bloom filter alone skips the file. I didn't find a Spark or parquet-java issue to link. -- 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]
