zeroshade commented on code in PR #1171:
URL: https://github.com/apache/arrow-go/pull/1171#discussion_r3807647511
##########
arrow/array/compare_test.go:
##########
@@ -613,6 +613,52 @@ func TestArrayEqualBaseArray(t *testing.T) {
}
}
+func TestArrayEqualFloatingPointSemantics(t *testing.T) {
+ negativeZero := math.Copysign(0, -1)
+ tests := []struct {
+ name string
+ left interface{}
+ right interface{}
+ want bool
+ }{
+ {"float16 signed zero", []float16.Num{float16.New(0)},
[]float16.Num{float16.New(float32(negativeZero))}, false},
+ {"float16 NaN",
[]float16.Num{float16.New(float32(math.NaN()))},
[]float16.Num{float16.New(float32(math.NaN()))}, true},
+ {"float32 signed zero", []float32{0},
[]float32{float32(negativeZero)}, true},
+ {"float32 NaN", []float32{float32(math.NaN())},
[]float32{float32(math.NaN())}, false},
+ {"float64 signed zero", []float64{0}, []float64{negativeZero},
true},
+ {"float64 NaN", []float64{math.NaN()}, []float64{math.NaN()},
false},
+ }
+
+ for _, test := range tests {
+ t.Run(test.name, func(t *testing.T) {
+ left := arrayOf(nil, test.left, nil)
+ defer left.Release()
+ right := arrayOf(nil, test.right, nil)
+ defer right.Release()
+
+ assert.Equal(t, test.want, array.Equal(left, right))
+ })
+ }
+}
+
+func TestArrayEqualFixedWidthIgnoresNullValues(t *testing.T) {
Review Comment:
This test has only three elements, so it takes the `Len() < 8` scalar branch
and never exercises the new nullable run/byte-comparison path it is intended to
protect.
Please use at least eight elements and add sliced arrays with independent,
non-byte-aligned offsets. The test should cover:
- differing bytes under null slots remain equal;
- a differing value in a valid run is unequal;
- runs crossing bitmap-byte or word boundaries;
- at least one temporal type such as `Timestamp`.
The existing benchmarks execute the bulk path, but the normal unit-test
suite currently has no direct coverage of it.
##########
arrow/array/numeric_generic.go:
##########
@@ -449,7 +451,44 @@ func NewDate64Data(data arrow.ArrayData) *Date64 {
func (a *Date64) Date64Values() []arrow.Date64 { return a.Values() }
-func arrayEqualFixedWidth[T arrow.FixedWidthType](left, right
arrow.TypedArray[T]) bool {
+type fixedWidthArray[T arrow.FixedWidthType] interface {
+ arrow.TypedArray[T]
+ Values() []T
+}
+
+func arrayEqualFixedWidth[T arrow.FixedWidthType](left, right
fixedWidthArray[T]) bool {
+ // Avoid the fixed cost of bytes.Equal for very small arrays.
+ if left.Len() < 8 {
+ return arrayEqualFixedWidthScalar(left, right)
+ }
+
+ leftValues := left.Values()
+ rightValues := right.Values()
+ if left.NullN() == 0 {
+ return bytes.Equal(arrow.GetBytes(leftValues),
arrow.GetBytes(rightValues))
+ }
+
+ runs := bitutils.NewSetBitRunReader(
Review Comment:
**Blocking:** the run reader panics when metadata reports partial nulls but
the validity buffer is empty.
This is reachable from malformed IPC: `loadCommon` fetches a validity buffer
whenever `NullCount() != 0`, while a zero-length buffer becomes
`memory.NewBufferBytes(nil)`. `validityBitmapEqual` explicitly handles this
empty-buffer case, but execution then reaches `NewSetBitRunReader`, which
slices the empty bitmap.
I reproduced this with an eight-element `Int64` array, `nullCount=1`, and a
zero-length validity buffer:
- merge base: returns a normal equality result
- this PR: panics in `baseSetBitRunReader.loadPartial`
Please avoid passing an empty bitmap to the run reader—either fall back to
the scalar comparison, as the previous implementation did, or reject the
malformed array earlier. Please add a regression test confirming `Equal` does
not panic.
--
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]
