laskoviymishka commented on code in PR #1909:
URL: https://github.com/apache/iceberg-go/pull/1909#discussion_r3874725664
##########
table/dv/roaring_bitmap.go:
##########
@@ -209,6 +209,19 @@ func (b *RoaringPositionBitmap) Serialize(w io.Writer)
error {
return nil
}
+// serializedSize returns the exact size of the portable bitmap encoding.
+// Empty buckets are omitted by Serialize and therefore do not contribute.
+func (b *RoaringPositionBitmap) serializedSize() int {
+ size := 8 // bitmap count
+ for _, bm := range b.bitmaps {
+ if bm.GetCardinality() > 0 {
+ size += 4 + int(bm.GetSerializedSizeInBytes()) //
bucket key + bitmap
Review Comment:
This `int(bm.GetSerializedSizeInBytes())` narrows a `uint64` to `int`, and
it's the one thing I'd want fixed before this lands.
On a 32-bit build any bucket serializing to ≥ 2 GiB flips negative, `size`
goes negative, and the `buf.Grow(...)` call in `SerializeDV` panics rather than
under-allocates. It's also the exact G115 pattern gosec flags, so it'll trip
the linter if that rule's on.
I'd keep the whole helper in `uint64` and convert once at the call site:
```go
func (b *RoaringPositionBitmap) serializedSize() uint64 {
size := uint64(8)
for _, bm := range b.bitmaps {
if bm.GetCardinality() > 0 {
size += 4 + bm.GetSerializedSizeInBytes()
}
}
return size
}
```
then guard the sum against `math.MaxInt` before the `int` conversion feeding
`buf.Grow`.
##########
table/dv/deletion_vector.go:
##########
@@ -121,22 +121,26 @@ func DeserializeDV(data []byte, expectedCardinality
int64) (*RoaringPositionBitm
func SerializeDV(bitmap *RoaringPositionBitmap) ([]byte, error) {
bitmap.RunLengthEncode()
- var bitmapBuf bytes.Buffer
- if err := bitmap.Serialize(&bitmapBuf); err != nil {
+ var buf bytes.Buffer
+ buf.Grow(dvLengthSize + dvMagicSize + bitmap.serializedSize() +
dvCRCSize)
+
+ var header [dvLengthSize + dvMagicSize]byte
+ binary.LittleEndian.PutUint32(header[dvLengthSize:], DVMagicNumber)
+ _, _ = buf.Write(header[:])
+
+ if err := bitmap.Serialize(&buf); err != nil {
return nil, fmt.Errorf("serialize roaring bitmap: %w", err)
}
- bitmapBytes := bitmapBuf.Bytes()
- innerLen := dvMagicSize + len(bitmapBytes)
- totalSize := dvLengthSize + innerLen + dvCRCSize
- out := make([]byte, totalSize)
+ bitmapDataEnd := buf.Len()
+ crc := crc32.ChecksumIEEE(buf.Bytes()[dvLengthSize:bitmapDataEnd])
- binary.BigEndian.PutUint32(out[0:dvLengthSize], uint32(innerLen))
-
binary.LittleEndian.PutUint32(out[dvLengthSize:dvLengthSize+dvMagicSize],
DVMagicNumber)
- copy(out[dvLengthSize+dvMagicSize:], bitmapBytes)
+ var trailer [dvCRCSize]byte
+ binary.BigEndian.PutUint32(trailer[:], crc)
+ _, _ = buf.Write(trailer[:])
- crc := crc32.ChecksumIEEE(out[dvLengthSize : totalSize-dvCRCSize])
- binary.BigEndian.PutUint32(out[totalSize-dvCRCSize:], crc)
+ out := buf.Bytes()
+ binary.BigEndian.PutUint32(out[:dvLengthSize],
uint32(bitmapDataEnd-dvLengthSize))
Review Comment:
This `uint32(bitmapDataEnd-dvLengthSize)` cast silently wraps if the bitmap
ever serializes past 4 GiB: the length prefix goes out wrong and
`DeserializeDV` later rejects the blob with a confusing length-mismatch instead
of a clean "too large" at write time.
It's not new (the old `uint32(innerLen)` had the same gap) and it's a narrow
path, but the rewrite is a natural spot to close it. I'd add a guard before the
patch:
```go
innerLen := bitmapDataEnd - dvLengthSize
if innerLen > math.MaxUint32 {
return nil, fmt.Errorf("deletion vector payload too large: %d bytes",
innerLen)
}
```
wdyt?
##########
table/dv/deletion_vector.go:
##########
@@ -121,22 +121,26 @@ func DeserializeDV(data []byte, expectedCardinality
int64) (*RoaringPositionBitm
func SerializeDV(bitmap *RoaringPositionBitmap) ([]byte, error) {
bitmap.RunLengthEncode()
- var bitmapBuf bytes.Buffer
- if err := bitmap.Serialize(&bitmapBuf); err != nil {
+ var buf bytes.Buffer
+ buf.Grow(dvLengthSize + dvMagicSize + bitmap.serializedSize() +
dvCRCSize)
+
+ var header [dvLengthSize + dvMagicSize]byte
+ binary.LittleEndian.PutUint32(header[dvLengthSize:], DVMagicNumber)
+ _, _ = buf.Write(header[:])
+
+ if err := bitmap.Serialize(&buf); err != nil {
return nil, fmt.Errorf("serialize roaring bitmap: %w", err)
}
- bitmapBytes := bitmapBuf.Bytes()
- innerLen := dvMagicSize + len(bitmapBytes)
- totalSize := dvLengthSize + innerLen + dvCRCSize
- out := make([]byte, totalSize)
+ bitmapDataEnd := buf.Len()
+ crc := crc32.ChecksumIEEE(buf.Bytes()[dvLengthSize:bitmapDataEnd])
- binary.BigEndian.PutUint32(out[0:dvLengthSize], uint32(innerLen))
-
binary.LittleEndian.PutUint32(out[dvLengthSize:dvLengthSize+dvMagicSize],
DVMagicNumber)
- copy(out[dvLengthSize+dvMagicSize:], bitmapBytes)
+ var trailer [dvCRCSize]byte
+ binary.BigEndian.PutUint32(trailer[:], crc)
+ _, _ = buf.Write(trailer[:])
- crc := crc32.ChecksumIEEE(out[dvLengthSize : totalSize-dvCRCSize])
- binary.BigEndian.PutUint32(out[totalSize-dvCRCSize:], crc)
+ out := buf.Bytes()
+ binary.BigEndian.PutUint32(out[:dvLengthSize],
uint32(bitmapDataEnd-dvLengthSize))
return out, nil
Review Comment:
Not blocking: I traced the output and it's byte-identical to the old path.
But since this rewrite reassembles the envelope (zero-fill + patch, magic via a
fixed array), the wire format is now only guarded by our own round-trip.
I'd add one test that runs `SerializeDV` on the same input as
`TestDeserializeDV` and asserts the bytes equal a Java-produced fixture (e.g.
`small-alternating-values-position-index.bin`). That pins the interop contract
so a future refactor can't drift the layout while still round-tripping cleanly
through our own reader. wdyt?
##########
table/dv/roaring_bitmap.go:
##########
@@ -209,6 +209,19 @@ func (b *RoaringPositionBitmap) Serialize(w io.Writer)
error {
return nil
}
+// serializedSize returns the exact size of the portable bitmap encoding.
Review Comment:
Small thing: the size is only exact once the inner bitmaps are
run-optimized, and `SerializeDV` guarantees that by calling `RunLengthEncode()`
first. Worth a sentence here so a future caller doesn't invoke this pre-RLE and
quietly under-size the `Grow`.
##########
table/dv/deletion_vector_bench_test.go:
##########
@@ -0,0 +1,55 @@
+// 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.
+
+package dv
+
+import "testing"
+
+var benchmarkSerializedDV []byte
+
+func BenchmarkSerializeDV(b *testing.B) {
+ for _, tt := range []struct {
+ name string
+ positions int
+ stride uint64
+ }{
+ {name: "sparse-1k", positions: 1_000, stride: 1_024},
+ {name: "sparse-100k", positions: 100_000, stride: 32},
+ {name: "sparse-1m", positions: 1_000_000, stride: 4},
Review Comment:
All three cases keep positions under 2^32, so everything lands in bucket 0
and the multi-bucket path in `serializedSize()` (the new map iteration) never
gets exercised here.
Could we add a case that straddles the boundary, e.g. positions at `0` and
`1<<32`, so the pre-size hint gets validated across bucket-key gaps too?
##########
table/dv/deletion_vector_bench_test.go:
##########
@@ -0,0 +1,55 @@
+// 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.
+
+package dv
+
+import "testing"
+
+var benchmarkSerializedDV []byte
+
+func BenchmarkSerializeDV(b *testing.B) {
+ for _, tt := range []struct {
+ name string
+ positions int
+ stride uint64
+ }{
+ {name: "sparse-1k", positions: 1_000, stride: 1_024},
+ {name: "sparse-100k", positions: 100_000, stride: 32},
+ {name: "sparse-1m", positions: 1_000_000, stride: 4},
+ } {
+ b.Run(tt.name, func(b *testing.B) {
+ bitmap := NewRoaringPositionBitmap()
+ for i := range tt.positions {
+ bitmap.Set(uint64(i) * tt.stride)
+ }
+
+ sample, err := SerializeDV(bitmap)
Review Comment:
Heads up that this warmup call mutates `bitmap` in place: `SerializeDV` runs
`RunLengthEncode()` on its argument, so by the time the timed loop hits it the
bitmap is already run-optimized and `RunLengthEncode` is a near-no-op every
iteration.
If measuring the steady-state re-serialize path is the intent, a one-line
comment saying so is enough; if you want the full encode-from-scratch cost,
build a fresh bitmap inside the loop. Either's fine, just worth being explicit
about which we're measuring.
--
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]
