You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "fallintoplace (via GitHub)" <gi...@apache.org> on 2026/06/30 21:56:04 UTC
[PR] Fix array data hashing coverage [arrow-go]
fallintoplace opened a new pull request, #872:
URL: https://github.com/apache/arrow-go/pull/872
### What changed
array.Hash now writes the array data type fingerprint, length, offset, every buffer with boundaries and nil markers, child data in order, and dictionary data when present.
### Why
The old implementation wrote length twice and only considered the null bitmap plus child data. That meant arrays with different value buffers, types, offsets, or dictionaries could produce the same hash.
### Validation
- go test ./arrow/array -run TestHashIncludesDataMetadataAndBuffers -count=1
- go test ./arrow/array -count=1
- PARQUET_TEST_DATA=/Users/hoangvu/Code/OSS/arrow-go/parquet-testing/data ARROW_TEST_DATA=/Users/hoangvu/Code/OSS/arrow-go/arrow-testing/data go test ./... -count=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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix array data hashing coverage [arrow-go]
Posted by "fallintoplace (via GitHub)" <gi...@apache.org>.
fallintoplace commented on code in PR #872:
URL: https://github.com/apache/arrow-go/pull/872#discussion_r3502180803
##########
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:
h.Write(b) is sufficient for hashing one isolated byte slice, but here the hash input is a structured stream made of multiple variable-width fields. maphash does not preserve Write call boundaries; it only sees the final sequence of bytes. So without length-prefixing, different structured inputs can serialize to the same byte stream, e.g. splitting bytes differently across adjacent buffers/strings
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix array data hashing coverage [arrow-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #872:
URL: https://github.com/apache/arrow-go/pull/872#discussion_r3502096949
##########
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:
why do we need to separately hash the length? isn't `h.Write(b)` sufficient?
##########
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))
+ h.Write(b)
+}
+
+func hashString(h *maphash.MapHash, s string) {
+ hashInt(h, len(s))
+ h.WriteString(s)
Review Comment:
same question as above, isn't `h.WriteString(s)` sufficient?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix array data hashing coverage [arrow-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #872:
URL: https://github.com/apache/arrow-go/pull/872#discussion_r3502272630
##########
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:
Can you construct an example of two valid arrays where this would happen?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix array data hashing coverage [arrow-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix array data hashing coverage [arrow-go]
Posted by "fallintoplace (via GitHub)" <gi...@apache.org>.
fallintoplace commented on PR #872:
URL: https://github.com/apache/arrow-go/pull/872#issuecomment-4858402079
@zeroshade Give me a minute, I will resync the branch and push this.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix array data hashing coverage [arrow-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on PR #872:
URL: https://github.com/apache/arrow-go/pull/872#issuecomment-4858366900
okay, just needs a rebase
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org