zeroshade commented on code in PR #1136:
URL: https://github.com/apache/arrow-go/pull/1136#discussion_r3752588233
##########
arrow/compute/exec/span.go:
##########
@@ -114,6 +114,10 @@ func (a *ArraySpan) UpdateNullCount() int64 {
if curNulls != array.UnknownNullCount {
return curNulls
}
+ if a.Buffers[0].Buf == nil {
Review Comment:
Consider `len(a.Buffers[0].Buf) == 0` rather than `== nil` — it covers
strictly more cases at no cost.
A non-nil but zero-length validity buffer is a real shape in this package:
`FillZeroLength` sets buffers to
`arrow.Uint64Traits.CastToBytes(span.Scratch[:])[:0]`, which is non-nil with
length 0. Such a span passes this `== nil` check and would still reach
`CountSetBits` — harmlessly today, since `Len` is 0 there, but the guard
wouldn't hold if a span ever carried an empty bitmap with a non-zero length.
There's precedent for both forms in this file: line 161 uses
`len(a.Buffers[0].Buf) == 0`, while lines 455 and 509 use `== nil`. The length
form is the safer of the two, and it makes the intent — "there is no bitmap
here" — independent of how the buffer happened to be zeroed.
##########
arrow/compute/exec/span_test.go:
##########
@@ -118,6 +118,10 @@ func TestArraySpan_UpdateNullCount(t *testing.T) {
want int64
}{
{"known", fields{Nulls: 25}, 25},
+ {"unknown without validity", fields{
Review Comment:
Nice — adding a row to the existing table rather than writing a standalone
test is the right instinct, and it's exactly what I suggested over on #1133 for
`TestFillZeroLengthListView`. Worth applying the same treatment there for
consistency, since both end up exercising this same file.
--
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]
