Repository navigation
fix(compute/exec): do not carry a cached null count across ArraySpan.SetSlice - #1306
Conversation
…SetSlice SetSlice kept the cached null count for the new slice when it was 0 or equal to the old length. That is only right while the count describes the whole span; a kernel that calls UpdateNullCount on the span the executor reuses stores the slice's count, and the next SetSlice then treats the following slice as all valid or all null. and_kleene and or_kleene return false where the answer is null once ExecCtx.ChunkSize is below the input length. Reset the count to unknown whenever a validity bitmap is present, as the C++ ArraySpan::SetSlice does, and add the chunk-size test.
zeroshade
left a comment
zeroshade
left a comment
There was a problem hiding this comment.
The null-count invalidation fix and focused chunk-size tests pass. One required release-audit fix remains: the new test file needs the standard ASF header.
|
Added the standard ASF header to arrow/compute/scalar_boolean_chunksize_test.go in the |
…eader added, awaiting re-review
zeroshade
left a comment
zeroshade
left a comment
There was a problem hiding this comment.
LGTM — the SetSlice invalidation fixes the reproduced chunk-size-dependent Kleene results for reused spans (matching the C++ ArraySpan::SetSlice behavior), and the previously requested ASF license header is now present.
Bitmap-counting trade-off (arrow/compute/exec/span.go:222)
Non-blocking observation: the conservative invalidation is correct for both cached zero and cached all-null counts. Those counts describe the previous active slice, not necessarily the next one. Preserving zero is safe when the requested range is known to belong to an all-valid parent, but ArraySpan currently stores no separate parent-range/null-count provenance. Simply retaining Nulls == 0 or Nulls == Len would reintroduce the bug. The executor still determines null-free output-bitmap elision from the original datums in setupPrealloc, independently of this change.
There is a measurable cost. In short local benchmarks with 1,048,576 all-valid values and allocated validity bitmaps, median and_kleene time went from 127.6 to 155.7 µs at ChunkSize=1024, and from 49.9 to 66.3 µs at ChunkSize=32768; corresponding or_kleene medians increased approximately 19% and 28%. Default-size Boolean runs differed by about −2% to +3%, and sliced Int64 add by +2% to +4%; allocation counts were unchanged. These were three 200 ms samples per case, not statistically isolated performance estimates. The extra work includes recounting sliced input bitmaps and recounting the final expanded output bitmap. A retained chunk-size benchmark would help guard this trade-off, but I would not preserve the unsafe cache to avoid it.
Smaller observations
- The new tests genuinely reproduce the defect: with the baseline/main
span.gosubstituted into the PR test tree, both addedSetSlicecases fail and both Kleene kernels fail at chunk sizes 1 and 2. All pass at this head, andgo test ./arrow/compute/... -count=1passes. - Additional local probes passed for cached-zero → all-null → valid transitions, nonzero offsets, identical slices, empty slices, bitmap-free spans, and the NULL type. NULL still receives the slice length; bitmap-free known-zero spans retain zero.
SetSliceremains a serial mutation of offset/length/count;UpdateNullCount's atomics do not make concurrent re-slicing safe. - CI has 28 successful checks, one skipped upload, and one failure: AMD64 Windows MinGW 64 CGO. Its
wcstod/wcstofredefinition errors match the unrelated main-branch toolchain problem fixed by PR #1369, not this change.
Prior review points
- ✅ Add the standard ASF header to the new test file rather than excluding it from RAT — addressed. This was the sole requested change in
zeroshade's prior review and inline thread. Commit0bc3fea955adds the header atarrow/compute/scalar_boolean_chunksize_test.go:1–15; no RAT exclusion was added. Both RC verification jobs now succeed.
This review was drafted by an AI-assisted tool and
confirmed by an Apache Arrow maintainer. The maintainer
approving this PR has read the findings and signed off. If
something feels off, please reply on the PR and a maintainer
will follow up.More on how Apache Arrow handles maintainer review:
CONTRIBUTING.md.
…null-count fix, commit 7090f9e; #1305 closed with it), named with the other merged Go work; apache/arrow#51222 has a contributor's fix pull request #52451 under review
Rationale for this change
and_kleeneandor_kleenereturnfalsewhere the answer is null whenExecCtx.ChunkSizeisbelow the input length (#1305).
ArraySpan.SetSlicekept the cached null count for the newslice when it was
0or equal to the old length. That is only right while the count describes thewhole span. The executor reuses one span per argument across slices, and a kernel that calls
UpdateNullCount()on it stores the slice's count, so the nextSetSlicetreats the followingslice as all valid or all null.
What changes are included in this PR?
SetSliceresets the count toUnknownNullCountwhenever a validity bitmap is present, as the C++ArraySpan::SetSlicedoes (cpp/src/arrow/array/data.h), and keeps it unknown when it was alreadynon-zero without a bitmap. The null type still gets the slice length; a span with no bitmap and a
zero count keeps zero. Two cases added to
TestArraySpan_SetSlice, and a new test that runs bothKleene kernels at chunk sizes 1, 2, 3, 4 and 8 against the default result.
Cost: a slice of an all-valid array with a bitmap now reports an unknown count, so a kernel that
asks pays one popcount over the slice instead of reading the cache. That is the C++ behaviour.
Are these changes tested?
Yes. The new chunk-size test fails on main (four assertions, chunk sizes 1 and 2 for both kernels)
and passes with the change;
go test ./arrow/compute/...passes.Are there any user-facing changes?
Results of
and_kleene/or_kleeneno longer depend onExecCtx.ChunkSize.ArraySpan.NullsafterSetSliceisUnknownNullCountin cases where it was previously0or the slice length; code thatread it directly should call
UpdateNullCount(), which is what the package's own kernels do.Closes #1305.