laskoviymishka commented on code in PR #1731:
URL: https://github.com/apache/iceberg-go/pull/1731#discussion_r3850549106
##########
catalog/rest/load_table_bench_test.go:
##########
@@ -81,7 +81,7 @@ func BenchmarkDecodeTableMetadata(b *testing.B) {
b.ResetTimer()
b.ReportAllocs()
- for i := 0; i < b.N; i++ {
+ for range b.N {
Review Comment:
This range-over-int cleanup is unrelated to the float-equality fix. I'd
split it into its own commit so a bisect on this change stays clean. Not
blocking.
##########
table/clustered_writer_test.go:
##########
@@ -55,8 +56,26 @@ func TestClusteredPartitionTrackingUsesComparableKeys(t
*testing.T) {
require.True(t, completed.contains(partitionRecord{[]byte{1, 2, 3}}))
require.True(t, completed.contains(partitionRecord{math.NaN()}))
+ require.True(t,
completed.contains(partitionRecord{math.Float64frombits(0x7ff8000000000002)}))
Review Comment:
This block stacks four distinct scenarios — cross-payload NaN equality,
float32 NaN, cross-width inequality, signed-zero distinctness — as flat
`require` calls, so a failure won't tell you which one broke. I'd wrap each in
a `t.Run` so failures point at the scenario. Non-blocking.
##########
table/partitioned_fanout_writer.go:
##########
@@ -75,35 +76,50 @@ type partitionExtractionPlan struct {
fields []partitionFieldInfo
}
-type binaryPartitionKey string
+type (
+ binaryPartitionKey string
+ float32PartitionKey uint32
+ float64PartitionKey uint64
+)
-type nanPartitionKey struct {
- bits int
-}
+const (
+ 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:
`bits` here (and in the float64 case below) shadows the `math/bits` import,
which this file uses in `initialPartitionRowCapacity` via `bits.Len64`. It's
inert since each case returns before touching the package, but `go vet -shadow`
and gocritic will flag it and it's a double-take right next to the import. I'd
rename both to `rawBits`.
##########
table/partitioned_fanout_writer.go:
##########
@@ -75,35 +76,50 @@ type partitionExtractionPlan struct {
fields []partitionFieldInfo
}
-type binaryPartitionKey string
+type (
+ binaryPartitionKey string
+ float32PartitionKey uint32
+ float64PartitionKey uint64
+)
-type nanPartitionKey struct {
- bits int
-}
+const (
+ canonicalFloat32NaNBits uint32 = 0x7fc00000
Review Comment:
Worth a short comment on these constants spelling out the two deliberate
choices, since both are behavioral.
Canonicalizing every NaN payload to `0x7fc00000` matches Java's
`floatToIntBits`, but it collapses `-NaN` and `+NaN`, which the spec's
sort-order table actually keeps distinct — following Java over the spec total
order is the right call here, just worth saying so.
And keeping signed zeros separate (`-0.0 ≠ +0.0`) flips the old `==` path
where they compared equal. It's spec-correct per `Float.compare`, but a
behavioral shift that I'd also call out in the PR description.
##########
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:
This swaps `partitionsMatch` to the new comparator, but
`matchEqualityDeletesToData` — its only caller — is dead code on current main.
The live equality-delete path is `buildEqualityDeleteIndex` +
`eqDeleteIndex.forDataFile` over in `equality_delete_index.go`. So the
fanout-writer half of this PR is genuinely active and fixed, but this half
corrects a dormant function and won't change any production scan.
The active path also normalizes floats differently:
`comparableEqualityDeletePartitionValue` promotes float32 to float64 bits and
uses one canonical NaN type across widths, so `float32(1.5)` matches
`float64(1.5)` there — while `comparablePartitionKey` keeps the widths
distinct. Under a float→double partition-spec evolution those two conventions
disagree.
Given the description says this "fixes equality-delete matching," I'd either
reword to say the scanner change future-proofs a dormant path, or apply the
same normalization to `comparableEqualityDeletePartitionValue` so the live
index path actually benefits — and settle which float-width convention is the
intended one. The added `TestMatchEqualityDeletesToDataHandlesFloatPartitions`
exercises the dormant function directly, so it passes without touching the
active path. wdyt?
##########
table/scanner_internal_test.go:
##########
@@ -285,6 +284,108 @@ func
TestEqualityDeletePartitionKeyDistinguishesSignedZero(t *testing.T) {
assert.NotEqual(t, negative, positive)
}
+func TestPartitionsMatchUsesFloatSemantics(t *testing.T) {
+ tests := []struct {
+ name string
+ left any
+ right any
+ match bool
+ }{
+ {name: "float32 NaN values", left: float32(math.NaN()), right:
float32(math.NaN()), match: true},
+ {name: "float64 NaN values", left: math.NaN(), right:
math.NaN(), match: true},
+ {name: "float32 NaN payloads", left:
math.Float32frombits(0x7fc00001), right: math.Float32frombits(0x7fc00002),
match: true},
+ {name: "float64 NaN payloads", left:
math.Float64frombits(0x7ff8000000000001), right:
math.Float64frombits(0x7ff8000000000002), match: true},
+ {name: "float32 signed zero", left: float32(math.Copysign(0,
-1)), right: float32(0), match: false},
+ {name: "float64 signed zero", left: math.Copysign(0, -1),
right: float64(0), match: false},
+ {name: "float widths differ", left: float32(1), right:
float64(1), match: false},
+ }
+
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ assert.Equal(t, tt.match,
+ partitionsMatch(map[int]any{1000: tt.left},
map[int]any{1000: tt.right}))
+ })
+ }
+}
+
+func TestMatchEqualityDeletesToDataHandlesBinaryPartitions(t *testing.T) {
+ dataSeqNum := int64(1)
+ deleteSeqNum := int64(2)
+ dataFile := &mockDataFile{
+ path: "data.parquet",
+ partition: map[int]any{1000: []byte{0xde, 0xad}},
+ }
+ matchingDelete := &mockDataFile{
+ path: "matching-delete.parquet",
+ partition: map[int]any{1000: append([]byte(nil), 0xde, 0xad)},
Review Comment:
The `append([]byte(nil), ...)` reads like a copy of something when it's
really just constructing a fresh slice. A plain `[]byte{0xde, 0xad}` literal
makes the same point — it's already a different backing array from the data
file's literal. Minor.
##########
table/scanner_internal_test.go:
##########
@@ -285,6 +284,108 @@ func
TestEqualityDeletePartitionKeyDistinguishesSignedZero(t *testing.T) {
assert.NotEqual(t, negative, positive)
}
+func TestPartitionsMatchUsesFloatSemantics(t *testing.T) {
+ tests := []struct {
+ name string
+ left any
+ right any
+ match bool
+ }{
+ {name: "float32 NaN values", left: float32(math.NaN()), right:
float32(math.NaN()), match: true},
+ {name: "float64 NaN values", left: math.NaN(), right:
math.NaN(), match: true},
+ {name: "float32 NaN payloads", left:
math.Float32frombits(0x7fc00001), right: math.Float32frombits(0x7fc00002),
match: true},
+ {name: "float64 NaN payloads", left:
math.Float64frombits(0x7ff8000000000001), right:
math.Float64frombits(0x7ff8000000000002), match: true},
+ {name: "float32 signed zero", left: float32(math.Copysign(0,
-1)), right: float32(0), match: false},
+ {name: "float64 signed zero", left: math.Copysign(0, -1),
right: float64(0), match: false},
+ {name: "float widths differ", left: float32(1), right:
float64(1), match: false},
Review Comment:
This table only exercises float and binary values. A pass-through case
(`int64(42)` matches, `string("a") != string("b")`) plus a `nil`-vs-`nil` case
would pin down the two branches that aren't float — especially relevant if we
switch to `==` above, since that's exactly where a non-comparable type would
surface. Cheap to add here.
##########
table/partitioned_fanout_writer.go:
##########
@@ -75,35 +76,50 @@ type partitionExtractionPlan struct {
fields []partitionFieldInfo
}
-type binaryPartitionKey string
+type (
+ binaryPartitionKey string
+ float32PartitionKey uint32
+ float64PartitionKey uint64
+)
-type nanPartitionKey struct {
- bits int
-}
+const (
+ 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)
if math.IsNaN(float64(value)) {
- return nanPartitionKey{bits: 32}
+ bits = canonicalFloat32NaNBits
}
+
+ return float32PartitionKey(bits)
case float64:
+ bits := math.Float64bits(value)
if math.IsNaN(value) {
- return nanPartitionKey{bits: 64}
+ bits = canonicalFloat64NaNBits
}
+
+ return float64PartitionKey(bits)
}
return value
}
+func partitionValuesEqual(left, right any) bool {
+ return reflect.DeepEqual(comparablePartitionKey(left),
comparablePartitionKey(right))
Review Comment:
I'd drop `reflect.DeepEqual` here for plain `==`.
Everything `comparablePartitionKey` returns is already a comparable type —
the named uint32/uint64 keys, `binaryPartitionKey` (string), or a pass-through
primitive — and the map-key paths (`getOrCreate`, `closedPartitionSet`) already
compare these with `==`. `reflect.DeepEqual` just adds reflection overhead and
makes the intent murkier than the `!=` it replaced.
The one thing `==` gives up is that it'll panic rather than quietly
mishandle a future non-comparable pass-through type. If that's a worry I'd
guard it in `comparablePartitionKey`'s default branch (or leave a one-line note
on the invariant) rather than paying for reflect on every call.
--
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]
