zeroshade commented on code in PR #1174:
URL: https://github.com/apache/arrow-go/pull/1174#discussion_r3808088171
##########
arrow/array/binary.go:
##########
@@ -418,17 +425,117 @@ func (a *LargeBinary) ValidateFull() error {
}
func arrayEqualLargeBinary(left, right *LargeBinary) bool {
- for i := 0; i < left.Len(); i++ {
- if left.IsNull(i) {
- continue
+ if useScalarVariableWidthEquality(left) {
+ for i := range left.Len() {
+ if !left.IsNull(i) && !bytes.Equal(left.Value(i),
right.Value(i)) {
+ return false
+ }
+ }
+ return true
+ }
+ return arrayEqualVariableWidth(
+ left.valueOffsets, right.valueOffsets,
+ left.valueBytes, right.valueBytes,
+ left.Offset(), right.Offset(), left.Len(),
+ left.NullN(), left.NullBitmapBytes(),
+ bytes.Equal,
+ )
+}
+
+type binaryOffset interface {
+ ~int32 | ~int64
+}
+
+func useScalarVariableWidthEquality(values arrow.Array) bool {
+ if values.NullN() == 0 {
+ return false
+ }
+
+ // Very short validity runs cost more to set up than direct value
comparisons.
Review Comment:
The sampling heuristic introduces a substantial regression for short arrays
with fragmented validity because it constructs and advances a run reader before
ultimately selecting the original scalar loop.
On Apple M4, alternating-null String arrays measured:
| Length | Merge base | Current | Regression |
|---:|---:|---:|---:|
| 4 | ~27 ns | ~68 ns | 2.5× |
| 8 | ~42 ns | ~100 ns | 2.4× |
| 16 | ~74 ns | ~115 ns | 55% |
| 32 | ~147 ns | ~187 ns | 27% |
| 64 | ~282 ns | ~315 ns | 11% |
The submitted benchmark only uses 65,536-element arrays, so it cannot expose
this fixed-cost regression. Please add a cheap length cutoff before sampling
and include short-array cases in the benchmark matrix. A performance change
should not make these common small-array comparisons materially slower.
##########
arrow/array/binary.go:
##########
@@ -418,17 +425,117 @@ func (a *LargeBinary) ValidateFull() error {
}
func arrayEqualLargeBinary(left, right *LargeBinary) bool {
- for i := 0; i < left.Len(); i++ {
- if left.IsNull(i) {
- continue
+ if useScalarVariableWidthEquality(left) {
+ for i := range left.Len() {
+ if !left.IsNull(i) && !bytes.Equal(left.Value(i),
right.Value(i)) {
+ return false
+ }
+ }
+ return true
+ }
+ return arrayEqualVariableWidth(
+ left.valueOffsets, right.valueOffsets,
+ left.valueBytes, right.valueBytes,
+ left.Offset(), right.Offset(), left.Len(),
+ left.NullN(), left.NullBitmapBytes(),
+ bytes.Equal,
+ )
+}
+
+type binaryOffset interface {
+ ~int32 | ~int64
+}
+
+func useScalarVariableWidthEquality(values arrow.Array) bool {
+ if values.NullN() == 0 {
+ return false
+ }
+
+ // Very short validity runs cost more to set up than direct value
comparisons.
+ // Sample a few runs and retain the scalar path when they average under
four values.
+ const (
+ sampleRuns = 8
+ minAverageRunLength = 4
+ )
+ runs := bitutils.NewSetBitRunReader(
Review Comment:
**Blocking:** `NullN() > 0` does not guarantee that a validity bitmap is
present. Malformed IPC or C Data can supply a nonzero declared null count with
an empty validity buffer.
This unconditionally constructs a run reader over that empty slice. I
reproduced equality on two eight-element Binary arrays with `nullCount=1` and
an empty validity buffer:
- merge base: returns normally
- this PR: panics in `baseSetBitRunReader.loadPartial`
An independent end-to-end IPC probe reproduced the same panic after changing
only the serialized `null_count` while leaving the validity buffer absent.
Please check `len(values.NullBitmapBytes())` before constructing the reader
and retain the scalar fallback when it is empty. Add a regression test
confirming equality does not panic on this malformed-input case.
--
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]
