This is an automated email from the ASF dual-hosted git repository.
zeroshade pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/arrow-go.git
The following commit(s) were added to refs/heads/main by this push:
new 7090f9ed fix(compute/exec): do not carry a cached null count across
ArraySpan.SetSlice (#1306)
7090f9ed is described below
commit 7090f9eddcd53a4909c401c7e33b6b567fa3f322
Author: singhpratech <[email protected]>
AuthorDate: Tue Oct 13 08:21:42 2026 -0400
fix(compute/exec): do not carry a cached null count across
ArraySpan.SetSlice (#1306)
### Rationale for this change
`and_kleene` and `or_kleene` return `false` where the answer is null
when `ExecCtx.ChunkSize` is
below the input length (#1305). `ArraySpan.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. The executor reuses one span per argument across slices, and
a kernel that calls
`UpdateNullCount()` on it stores the slice's count, so the next
`SetSlice` treats the following
slice as all valid or all null.
### What changes are included in this PR?
`SetSlice` resets the count to `UnknownNullCount` whenever a validity
bitmap is present, as the C++
`ArraySpan::SetSlice` does (`cpp/src/arrow/array/data.h`), and keeps it
unknown when it was already
non-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 both
Kleene 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_kleene` no longer depend on
`ExecCtx.ChunkSize`. `ArraySpan.Nulls` after
`SetSlice` is `UnknownNullCount` in cases where it was previously `0` or
the slice length; code that
read it directly should call `UpdateNullCount()`, which is what the
package's own kernels do.
Closes #1305.
---
arrow/compute/exec/span.go | 12 ++--
arrow/compute/exec/span_test.go | 2 +
arrow/compute/scalar_boolean_chunksize_test.go | 76 ++++++++++++++++++++++++++
3 files changed, 84 insertions(+), 6 deletions(-)
diff --git a/arrow/compute/exec/span.go b/arrow/compute/exec/span.go
index 49350eeb..d071c0c6 100644
--- a/arrow/compute/exec/span.go
+++ b/arrow/compute/exec/span.go
@@ -214,13 +214,13 @@ func (a *ArraySpan) SetSlice(off, length int64) {
return
}
+ // The cached null count describes the span as it was, not the slice.
+ // As in the C++ ArraySpan::SetSlice, a slice of anything with a
validity
+ // bitmap has an unknown count until someone asks for it; only the null
+ // type and a span without a bitmap can be counted without looking.
if a.Type.ID() != arrow.NULL {
- if a.Nulls != 0 {
- if a.Nulls == a.Len {
- a.Nulls = length
- } else {
- a.Nulls = array.UnknownNullCount
- }
+ if a.Nulls != 0 || len(a.Buffers[0].Buf) != 0 {
+ a.Nulls = array.UnknownNullCount
}
} else {
a.Nulls = length
diff --git a/arrow/compute/exec/span_test.go b/arrow/compute/exec/span_test.go
index 90d4c93e..3a19b21c 100644
--- a/arrow/compute/exec/span_test.go
+++ b/arrow/compute/exec/span_test.go
@@ -523,6 +523,8 @@ func TestArraySpan_SetSlice(t *testing.T) {
{"null type", fields{Type: arrow.Null}, args{5, 10}, 10},
{"not-null type", fields{Type: arrow.PrimitiveTypes.Int8},
args{5, 10}, 0},
{"not-null type with nulls", fields{Type:
arrow.PrimitiveTypes.Int8, Nulls: -1}, args{5, 10}, array.UnknownNullCount},
+ {"no nulls but a validity bitmap", fields{Type:
arrow.PrimitiveTypes.Int8, Len: 16, Buffers: [3]exec.BufferSpan{{Buf:
[]byte{0xff, 0xff}}}}, args{5, 10}, array.UnknownNullCount},
+ {"all null before the slice", fields{Type:
arrow.PrimitiveTypes.Int8, Len: 16, Nulls: 16, Buffers:
[3]exec.BufferSpan{{Buf: []byte{0, 0}}}}, args{5, 10}, array.UnknownNullCount},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
diff --git a/arrow/compute/scalar_boolean_chunksize_test.go
b/arrow/compute/scalar_boolean_chunksize_test.go
new file mode 100644
index 00000000..61c63a1c
--- /dev/null
+++ b/arrow/compute/scalar_boolean_chunksize_test.go
@@ -0,0 +1,76 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package compute_test
+
+import (
+ "context"
+ "testing"
+
+ "github.com/apache/arrow-go/v18/arrow"
+ "github.com/apache/arrow-go/v18/arrow/array"
+ "github.com/apache/arrow-go/v18/arrow/compute"
+ "github.com/apache/arrow-go/v18/arrow/memory"
+)
+
+// and_kleene and or_kleene over two boolean arrays with nulls, executed with
+// different ExecCtx.ChunkSize values; the result must not depend on ChunkSize.
+func TestKleeneChunkSizeIndependent(t *testing.T) {
+ mem := memory.NewCheckedAllocator(memory.DefaultAllocator)
+ defer mem.AssertSize(t, 0)
+
+ // left: [true, null, true, null, false, true, null, true]
+ // right: [null, true, true, false, null, null, true, true]
+ lb := array.NewBooleanBuilder(mem)
+ lb.AppendValues([]bool{true, false, true, false, false, true, false,
true}, []bool{true, false, true, false, true, true, false, true})
+ left := lb.NewArray()
+ lb.Release()
+ defer left.Release()
+ rb := array.NewBooleanBuilder(mem)
+ rb.AppendValues([]bool{false, true, true, false, false, false, true,
true}, []bool{false, true, true, true, false, false, true, true})
+ right := rb.NewArray()
+ rb.Release()
+ defer right.Release()
+
+ for _, fn := range []string{"and_kleene", "or_kleene"} {
+ t.Run(fn, func(t *testing.T) { checkChunkSizeIndependent(t,
mem, fn, left, right) })
+ }
+}
+
+func checkChunkSizeIndependent(t *testing.T, mem memory.Allocator, fn string,
left, right arrow.Array) {
+ run := func(chunk int64) string {
+ ectx := compute.DefaultExecCtx()
+ ectx.ChunkSize = chunk
+ ctx :=
compute.SetExecCtx(compute.WithAllocator(context.Background(), mem), ectx)
+ out, err := compute.CallFunction(ctx, fn, nil,
&compute.ArrayDatum{Value: left.Data()}, &compute.ArrayDatum{Value:
right.Data()})
+ if err != nil {
+ t.Fatal(err)
+ }
+ defer out.Release()
+ arr := out.(*compute.ArrayDatum).MakeArray()
+ defer arr.Release()
+ return arr.String()
+ }
+ ref := run(compute.DefaultMaxChunkSize)
+ t.Logf("ChunkSize default: %s", ref)
+ for _, n := range []int64{1, 2, 3, 4, 8} {
+ got := run(n)
+ t.Logf("ChunkSize %d: %s", n, got)
+ if got != ref {
+ t.Errorf("ChunkSize %d: got %s, want %s", n, got, ref)
+ }
+ }
+}
