perf(arrow/array): compare list values by valid runs - #1184
Conversation
e5b53f1 to
0b3b3cb
Compare
zeroshade
left a comment
There was a problem hiding this comment.
The valid-run optimization looks good overall and CI is green. I found one behavior regression inline that should be fixed before merge.
This review was drafted by an AI-assisted tool and confirmed by an Apache Arrow Go maintainer. The finding below is an observation, not a blocking review; an Apache Arrow Go maintainer — a real person — will take the next look at the PR.
See the Apache Arrow Go contributing guide.
|
|
||
| func arrayEqualListOffsets[T listOffset](leftValues, rightValues arrow.Array, | ||
| leftOffsets, rightOffsets []T, leftOffset, rightOffset, length int, validBits []byte) bool { | ||
| return bitutils.VisitSetBitRuns(validBits, int64(leftOffset), int64(length), |
There was a problem hiding this comment.
IsNull and IsValid treat both nil and a zero-length validity buffer as all-valid, but VisitSetBitRuns special-cases only nil. Consequently, a nonempty list created through NewData with a non-nil empty validity buffer now panics here when it is the left operand, while reversing the operands returns true. The previous implementation returned true in both directions.
Although Validate rejects this array shape, this is still a regression in Equal behavior for data constructible through the exported API. Please normalize len(validBits) == 0 to the all-valid path, apply the same handling in arrayEqualFixedSizeList, and add regression coverage for both operand orders.
What does this PR do?
ListView and LargeListView are not included because their child ranges can be non-contiguous.
Why?
List equality currently creates two temporary child arrays and recursively calls
Equalfor every valid parent value.For 65,536 all-valid lists, that creates about 131,000 temporary child arrays. A valid-run comparison checks the parent lengths and compares one contiguous child range instead.
This follows the same approach as Arrow C++ list equality.
Benchmarks
Apple M1 Pro, 65,536 equal parent values:
Alternating valid and null parents stays around 11.2 ms in both versions. This is the most fragmented case, where each valid run contains one parent value.
Tests
go test ./arrow/...go test -race ./arrow/arraygo vet ./arrow/array