cloud-fan commented on code in PR #56146:
URL: https://github.com/apache/spark/pull/56146#discussion_r3313415321
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/NormalizeFloatingNumbers.scala:
##########
@@ -28,36 +28,30 @@ import org.apache.spark.sql.types._
import org.apache.spark.util.ArrayImplicits._
/**
- * We need to take care of special floating numbers (NaN and -0.0) in several
places:
- * 1. When compare values, different NaNs should be treated as same, `-0.0`
and `0.0` should be
- * treated as same.
- * 2. In aggregate grouping keys, different NaNs should belong to the same
group, `-0.0` and `0.0`
- * should belong to the same group.
- * 3. In join keys, different NaNs should be treated as same, `-0.0` and
`0.0` should be
- * treated as same.
- * 4. In window partition keys, different NaNs should belong to the same
partition, `-0.0`
- * and `0.0` should belong to the same partition.
- * 5. In hash-based array set operations, different NaNs should be treated
as same, `-0.0`
- * and `0.0` should be treated as same.
+ * Certain pairs of floating point numbers require special handling:
+ * 1. 0.0 / -0.0
+ * 2. NaN / NaN
+ * That's because we want to treat each of these pairs of numbers as equal,
even though they
+ * have different binary representations. (IEEE 754 allows multiple distinct
bit patterns for
+ * NaN.)
*
- * Case 1 is fine, as we handle NaN and `-0.0` well during comparison. For
complex types, we
- * recursively compare the fields/elements, so it's also fine.
+ * This special handling is required in several places:
+ * 1. When comparing values
+ * 2. When grouping keys for aggregates
+ * 3. When joining keys
+ * 4. When partitioning keys for windows
+ * 5. When executing array set operations
*
- * Case 2, 3 and 4 are problematic, as Spark SQL turns grouping/join/window
partition keys into
- * binary `UnsafeRow` and compare the binary data directly. Different NaNs
have different binary
- * representation, and the same thing happens for `-0.0` and `0.0`.
- *
- * Case 5 is problematic for a similar reason: hash-based array set operations
compare elements by
- * their binary representation via hash sets.
+ * Case 1 is already handled in [[SQLOrderingUtil]] and
[[CodeGenerator.genEqual]].
+ * Cases 2-5 are handled by this optimizer rule.
Review Comment:
"Cases 2-5 are handled by this optimizer rule" overstates what the rule
does. Case 2 (aggregate grouping) is **not** handled here:
- The TODO at L87 of this same file says so ("For now we normalize grouping
expressions in `AggUtils` during planning").
- The actual normalization happens in `SparkStrategies` (around L472), with
the comment: "Ideally this should be done in `NormalizeFloatingNumbers`, but we
do it here because `groupingExpressions` is not extracted during logical phase."
Suggest splitting case 2 out, e.g.:
```suggestion
* Case 1 is already handled in [[SQLOrderingUtil]] and
[[CodeGenerator.genEqual]].
* Cases 3, 4, and 5 are handled by this optimizer rule.
* Case 2 is handled separately during planning in `SparkStrategies` (see
the TODO below).
```
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/NormalizeFloatingNumbers.scala:
##########
@@ -28,36 +28,30 @@ import org.apache.spark.sql.types._
import org.apache.spark.util.ArrayImplicits._
/**
- * We need to take care of special floating numbers (NaN and -0.0) in several
places:
- * 1. When compare values, different NaNs should be treated as same, `-0.0`
and `0.0` should be
- * treated as same.
- * 2. In aggregate grouping keys, different NaNs should belong to the same
group, `-0.0` and `0.0`
- * should belong to the same group.
- * 3. In join keys, different NaNs should be treated as same, `-0.0` and
`0.0` should be
- * treated as same.
- * 4. In window partition keys, different NaNs should belong to the same
partition, `-0.0`
- * and `0.0` should belong to the same partition.
- * 5. In hash-based array set operations, different NaNs should be treated
as same, `-0.0`
- * and `0.0` should be treated as same.
+ * Certain pairs of floating point numbers require special handling:
+ * 1. 0.0 / -0.0
+ * 2. NaN / NaN
+ * That's because we want to treat each of these pairs of numbers as equal,
even though they
+ * have different binary representations. (IEEE 754 allows multiple distinct
bit patterns for
+ * NaN.)
*
- * Case 1 is fine, as we handle NaN and `-0.0` well during comparison. For
complex types, we
- * recursively compare the fields/elements, so it's also fine.
+ * This special handling is required in several places:
+ * 1. When comparing values
+ * 2. When grouping keys for aggregates
+ * 3. When joining keys
Review Comment:
Nit: "When joining keys" parses awkwardly compared to the sibling bullets
("When comparing values", "When grouping keys for aggregates", "When
partitioning keys for windows"). Keys aren't really the object of *join*.
```suggestion
* 3. When joining on keys
```
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/NormalizeFloatingNumbers.scala:
##########
@@ -28,36 +28,30 @@ import org.apache.spark.sql.types._
import org.apache.spark.util.ArrayImplicits._
/**
- * We need to take care of special floating numbers (NaN and -0.0) in several
places:
- * 1. When compare values, different NaNs should be treated as same, `-0.0`
and `0.0` should be
- * treated as same.
- * 2. In aggregate grouping keys, different NaNs should belong to the same
group, `-0.0` and `0.0`
- * should belong to the same group.
- * 3. In join keys, different NaNs should be treated as same, `-0.0` and
`0.0` should be
- * treated as same.
- * 4. In window partition keys, different NaNs should belong to the same
partition, `-0.0`
- * and `0.0` should belong to the same partition.
- * 5. In hash-based array set operations, different NaNs should be treated
as same, `-0.0`
- * and `0.0` should be treated as same.
+ * Certain pairs of floating point numbers require special handling:
+ * 1. 0.0 / -0.0
+ * 2. NaN / NaN
+ * That's because we want to treat each of these pairs of numbers as equal,
even though they
+ * have different binary representations. (IEEE 754 allows multiple distinct
bit patterns for
+ * NaN.)
*
- * Case 1 is fine, as we handle NaN and `-0.0` well during comparison. For
complex types, we
- * recursively compare the fields/elements, so it's also fine.
+ * This special handling is required in several places:
+ * 1. When comparing values
+ * 2. When grouping keys for aggregates
+ * 3. When joining keys
+ * 4. When partitioning keys for windows
+ * 5. When executing array set operations
*
- * Case 2, 3 and 4 are problematic, as Spark SQL turns grouping/join/window
partition keys into
- * binary `UnsafeRow` and compare the binary data directly. Different NaNs
have different binary
- * representation, and the same thing happens for `-0.0` and `0.0`.
- *
- * Case 5 is problematic for a similar reason: hash-based array set operations
compare elements by
- * their binary representation via hash sets.
+ * Case 1 is already handled in [[SQLOrderingUtil]] and
[[CodeGenerator.genEqual]].
Review Comment:
Neither `SQLOrderingUtil` nor `CodeGenerator` is imported in this file (only
`CodegenContext, ExprCode` are imported from the codegen package), so these
wiki-style links won't resolve in generated Scaladoc. Spark's convention for
cross-package links is to fully qualify, e.g. in `InternalRow.scala`,
`subquery.scala`, `QueryCompilationErrors.scala`.
```suggestion
* Case 1 is already handled in
[[org.apache.spark.sql.catalyst.util.SQLOrderingUtil]] and
*
[[org.apache.spark.sql.catalyst.expressions.codegen.CodeGenerator.genEqual]].
```
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/NormalizeFloatingNumbers.scala:
##########
@@ -28,36 +28,30 @@ import org.apache.spark.sql.types._
import org.apache.spark.util.ArrayImplicits._
/**
- * We need to take care of special floating numbers (NaN and -0.0) in several
places:
- * 1. When compare values, different NaNs should be treated as same, `-0.0`
and `0.0` should be
- * treated as same.
- * 2. In aggregate grouping keys, different NaNs should belong to the same
group, `-0.0` and `0.0`
- * should belong to the same group.
- * 3. In join keys, different NaNs should be treated as same, `-0.0` and
`0.0` should be
- * treated as same.
- * 4. In window partition keys, different NaNs should belong to the same
partition, `-0.0`
- * and `0.0` should belong to the same partition.
- * 5. In hash-based array set operations, different NaNs should be treated
as same, `-0.0`
- * and `0.0` should be treated as same.
+ * Certain pairs of floating point numbers require special handling:
+ * 1. 0.0 / -0.0
+ * 2. NaN / NaN
+ * That's because we want to treat each of these pairs of numbers as equal,
even though they
+ * have different binary representations. (IEEE 754 allows multiple distinct
bit patterns for
+ * NaN.)
*
- * Case 1 is fine, as we handle NaN and `-0.0` well during comparison. For
complex types, we
- * recursively compare the fields/elements, so it's also fine.
+ * This special handling is required in several places:
+ * 1. When comparing values
+ * 2. When grouping keys for aggregates
+ * 3. When joining keys
+ * 4. When partitioning keys for windows
+ * 5. When executing array set operations
*
- * Case 2, 3 and 4 are problematic, as Spark SQL turns grouping/join/window
partition keys into
- * binary `UnsafeRow` and compare the binary data directly. Different NaNs
have different binary
- * representation, and the same thing happens for `-0.0` and `0.0`.
- *
- * Case 5 is problematic for a similar reason: hash-based array set operations
compare elements by
- * their binary representation via hash sets.
+ * Case 1 is already handled in [[SQLOrderingUtil]] and
[[CodeGenerator.genEqual]].
+ * Cases 2-5 are handled by this optimizer rule.
Review Comment:
The original docstring's value wasn't just listing the cases - it explained
*why* each kind needs normalization: cases 2/3/4 because Spark compares
grouping/join/window keys as binary `UnsafeRow` bytes, and case 5 because
hash-based set ops compare elements by their binary representation via hash
sets. The rewrite drops both rationales. A future reader can now see *what* is
normalized but not *why* this lives in a logical-plan rule rather than (say)
`EqualTo`'s eval. The paragraph below mentions `UnsafeRow` only as the
motivation for "ideally we'd do this in physical operators", and never covers
hash-set semantics for the array-set ops case.
Consider folding a one-line "why" back in - e.g. after "This special
handling is required in several places:", a sentence like: "Spark compares
grouping/join/window keys by their binary `UnsafeRow` representation, and
hash-based array set ops compare elements by their binary representation via
hash sets; both bypass the equality logic from case 1."
--
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]
