zeroshade commented on code in PR #928:
URL: https://github.com/apache/arrow-go/pull/928#discussion_r3566763204
##########
arrow/scalar/scalar.go:
##########
@@ -1063,8 +1067,26 @@ func Hash(seed maphash.Seed, s Scalar) uint64 {
case TemporalScalar:
return valueHash(s.value())
case ListScalar:
- array.Hash(&h, s.GetList().Data())
- hash()
+ list := s.GetList()
+ hashUint64(uint64(list.Len()))
+ for i := 0; i < list.Len(); i++ {
Review Comment:
The fix correctly restores the equal-values-hash-equal contract, but the
aggregation is order-insensitive and cancellation-prone: each element is folded
via the `hash()` closure (`out ^= h.Sum64(); h.Reset()`), so element
contributions XOR commutatively. As a result `[1, 2]` and `[2, 1]` hash
identically (even though list equality is order-sensitive), and any
repeated-value list cancels — e.g. `[x, x]` collapses to `typeHash ^
lenHash(2)` for every `x`, so `[1,1]`, `[2,2]`, ... all collide. It's not a
correctness violation (collisions are permitted), but it's an avoidable
systematic-collision regression for a hash function. Consider folding the whole
sequence into a single running `maphash` state (no per-element `Reset`), or
mixing the element index in before each value. Worth adding negative tests:
`[1,2]` vs `[2,1]`, `[1,1]` vs `[2,2]`, and `[null,1]` vs `[1,null]`.
--
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]
