laskoviymishka commented on code in PR #1731:
URL: https://github.com/apache/iceberg-go/pull/1731#discussion_r3872728266
##########
table/scanner.go:
##########
@@ -609,7 +609,7 @@ func matchEqualityDeletesToData(dataEntry
iceberg.ManifestEntry, eqDeleteEntries
}
func partitionsMatch(a, b map[int]any) bool {
- return maps.EqualFunc(a, b, reflect.DeepEqual)
+ return maps.EqualFunc(a, b, partitionValuesEqual)
Review Comment:
Good — routing this through `partitionValuesEqual` unifies the float
convention with the writer, which is what I was after last round.
One thing that's still uneven: `partitionValuesEqual` compares integer
partition values with a raw `==`, so `int32(7)` and `int64(7)` come out unequal
here, whereas the active index path (`comparableEqualityDeletePartitionValue`)
normalizes them to a common width and treats them as equal. Since
`matchEqualityDeletesToData` is the only caller of `partitionsMatch` and it's
still dead code, this doesn't bite production — but the two paths still
disagree, just on ints now instead of floats.
I'd either normalize integers in `partitionValuesEqual` too, or drop a short
comment on `matchEqualityDeletesToData` noting it's a dormant reference impl
and the index path is the live one. The PR description still reads as though
this is the production path. wdyt?
##########
table/scanner_internal_test.go:
##########
@@ -232,12 +232,39 @@ func TestEqualityDeletePartitionKeyNormalizesValues(t
*testing.T) {
name string
left any
right any
+ equal bool
}{
- {name: "integer widths", left: int32(7), right: int64(7)},
- {name: "date and integer", left: iceberg.Date(7), right:
int32(7)},
+ {name: "integer widths", left: int32(7), right: int64(7),
equal: true},
+ {name: "date and integer", left: iceberg.Date(7), right:
int32(7), equal: true},
{name: "float widths", left: float32(1.5), right: float64(1.5)},
Review Comment:
This case has no `equal` field now, so it's silently asserting
`float32(1.5)` and `float64(1.5)` are distinct — which is correct, but the name
gives no hint that's the intent.
Your new `TestEqualityDeleteIndexUsesFloatSemantics` already names the
equivalent case `float widths stay distinct`; I'd match that here.
##########
table/partitioned_fanout_writer.go:
##########
@@ -75,35 +75,54 @@ type partitionExtractionPlan struct {
fields []partitionFieldInfo
}
-type binaryPartitionKey string
+type (
+ binaryPartitionKey string
+ float32PartitionKey uint32
+ float64PartitionKey uint64
+)
-type nanPartitionKey struct {
- bits int
-}
+const (
+ // Canonical NaN payloads make all NaNs of the same width compare equal.
+ // The width-specific key types and raw bits preserve float width and
signed zero.
+ canonicalFloat32NaNBits uint32 = 0x7fc00000
+ canonicalFloat64NaNBits uint64 = 0x7ff8000000000000
+)
func comparablePartitionKey(value any) any {
switch value := value.(type) {
case []byte:
return binaryPartitionKey(value)
case float32:
+ bits := math.Float32bits(value)
Review Comment:
This is the rename I flagged last round, and it's live: `math/bits` is
imported (line 26) and used as `bits.Len64` in `initialPartitionRowCapacity`,
so these `bits` locals shadow the package inside the float cases.
It compiles today because the usage is in a different function, but the
shadow linter will trip and anyone adding a `math/bits` call to these cases
gets a confusing error. I'd rename both here and in the float64 case at line
103 to something like `rawBits`.
##########
table/equality_delete_index.go:
##########
@@ -106,19 +103,10 @@ func comparableEqualityDeletePartitionValue(value any)
(any, error) {
return equalityDeleteIntegerPartitionValue(value), nil
case iceberg.TimestampNano:
return equalityDeleteIntegerPartitionValue(value), nil
- case float32:
- value64 := float64(value)
- if math.IsNaN(value64) {
- return equalityDeleteNaNPartitionValue{}, nil
- }
-
- return
equalityDeleteFloatPartitionValue(math.Float64bits(value64)), nil
- case float64:
- if math.IsNaN(value) {
- return equalityDeleteNaNPartitionValue{}, nil
- }
-
- return
equalityDeleteFloatPartitionValue(math.Float64bits(value)), nil
+ case float32, float64:
+ // Keep equality-delete indexing aligned with writer partition
keys:
+ // canonicalize NaNs within each width and preserve signed zero.
+ return comparablePartitionKey(value), nil
Review Comment:
Nice — delegating to `comparablePartitionKey` is exactly the unification I
wanted.
Not for this PR, but `comparablePartitionKey` and `partitionValuesEqual` are
now shared partition-equality primitives that live in
`partitioned_fanout_writer.go`, and this reader-side file reaches across into
the writer file to use them. A follow-up that pulls them into something like
`partition_equality.go` would read more naturally. Non-blocking.
--
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]
