zeroshade commented on code in PR #872:
URL: https://github.com/apache/arrow-go/pull/872#discussion_r3507907113
##########
arrow/array/data.go:
##########
@@ -270,12 +268,59 @@ func NewSliceData(data arrow.ArrayData, i, j int64)
arrow.ArrayData {
func Hash(h *maphash.MapHash, data arrow.ArrayData) {
a := data.(*Data)
- h.Write((*[bits.UintSize / 8]byte)(unsafe.Pointer(&a.length))[:])
- h.Write((*[bits.UintSize / 8]byte)(unsafe.Pointer(&a.length))[:])
- if len(a.buffers) > 0 && a.buffers[0] != nil {
- h.Write(a.buffers[0].Bytes())
+ hashString(h, a.dtype.Fingerprint())
+ hashInt(h, a.length)
+ hashInt(h, a.offset)
+
+ hashInt(h, len(a.buffers))
+ for _, b := range a.buffers {
+ if b == nil {
+ hashByte(h, 0)
+ continue
+ }
+
+ hashByte(h, 1)
+ hashBytes(h, b.Bytes())
}
+
+ hashInt(h, len(a.childData))
for _, c := range a.childData {
+ if c == nil {
+ hashByte(h, 0)
+ continue
+ }
+
+ hashByte(h, 1)
Hash(h, c)
}
+
+ if a.dictionary == nil {
+ hashByte(h, 0)
+ return
+ }
+
+ hashByte(h, 1)
+ Hash(h, a.dictionary)
+}
+
+func hashByte(h *maphash.MapHash, v byte) {
+ var b [1]byte
+ b[0] = v
+ h.Write(b[:])
+}
+
+func hashInt(h *maphash.MapHash, v int) {
+ var b [8]byte
+ binary.LittleEndian.PutUint64(b[:], uint64(int64(v)))
+ h.Write(b[:])
+}
+
+func hashBytes(h *maphash.MapHash, b []byte) {
+ hashInt(h, len(b))
Review Comment:
I went and verified this — you're right, the length prefix is needed in
general. Here's a concrete pair of `binary` arrays that both pass
`ValidateFull` and hold **different** values, yet hash identically without it:
| | length | validity | offsets (int32) | values | value[0] |
|---|---|---|---|---|---|
| A | 1 | nil | `[0, 1]` | `00 00 00 01 58` | `0x00` |
| B | 1 | nil | `[0, 1, 1]` | `58` | `0x58` |
Both are valid — only `offsets[:length+1]` and `values[:lastOffset]` are
read, so the extra trailing bytes are allowed. Without the length prefix, each
one's buffer section serializes to the identical stream:
```
00 01 00 00 00 00 01 00 00 00 01 00 00 00 01 58
```
A parses that as `[nil][offsets 0,1][values 00 00 00 01 58]` and B as
`[nil][offsets 0,1,1][values 58]` — same bytes, so they collide for every seed.
The presence byte doesn't help here; it's just another `0x01` that can also
occur inside a buffer.
You're right that fresh **builder** output won't trigger this: builders trim
buffers to canonical sizes, so the offsets buffer pins the split (I checked
`["AB","C"]` vs `["A","BC"]`, and the `list<int8>` equivalent — no collision).
But the over-long buffers above are exactly what `Hash` has to accept from
slicing, IPC / C-Data import, and manual `NewData`, and the caller
(`scalar.Hash` -> `array.Hash(s.GetList().Data())`) hands us arbitrary
list-scalar data. I'd rather the hash not depend on "every buffer is
canonically sized."
It also lines up with arrow-c++ and arrow-rs: the C++ `RowEncoder` frames
variable-width values as `[null byte, length, bytes]`, and the arrow-rs row
format is explicitly designed so a byte is never ambiguously part of a value vs
a delimiter.
Let's keep the length prefix.
--
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]
