Skip to content

perf(arrow/array): concatenate bitmaps directly - #1224

Open
fallintoplace wants to merge 2 commits into
apache:mainfrom
fallintoplace:perf/arrow-bitmap-concat
Open

perf(arrow/array): concatenate bitmaps directly#1224
fallintoplace wants to merge 2 commits into
apache:mainfrom
fallintoplace:perf/arrow-bitmap-concat

Conversation

@fallintoplace

Copy link
Copy Markdown
Contributor

What does this change?

  • concat used to build a temporary []bitmap for each bitmap it concatenated.
  • concatBitmaps now reads the input ArrayData directly by buffer index.
  • Keep the existing nil bitmap behavior for all-valid inputs and preserve sliced bitmap offsets.

Benchmark

Command:

go test ./arrow/array -run '^$' -bench '^BenchmarkConcatenateBitmaps$' -benchmem -benchtime=200ms -count=3

Apple M1 Pro, Go 1.26.3. The benchmark concatenates 65,536 rows split across 64, 1,024, and 8,192 chunks.

Input Chunks Before ns/op After ns/op Before B/op After B/op Allocs before/after
nullable Boolean 64 6,965 5,689 25,824 20,448 10 / 8
nullable Boolean 1,024 55,375 41,568 119,648 37,728 10 / 8
nullable Boolean 8,192 412,768 276,285 805,730 150,368 10 / 8
nullable int32 8,192 313,516 269,165 738,913 411,233 9 / 8

Tests

  • go test ./arrow/array -count=1
  • go test ./arrow/... ./internal/...

@zeroshade zeroshade left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The performance and allocation improvements reproduce, and the normal-input correctness, race, vet, LSP, and CI checks pass. One error-path allocation regression needs correction before merge.

Comment thread arrow/array/concat.go
if bm.data == nil { // if the bitmap is nil, that implies that the value is true for all elements
bitutil.SetBitsTo(out.Bytes(), int64(offset), int64(bm.rng.len), true)
for _, d := range data {
if buf := d.Buffers()[idx]; buf == nil { // if the bitmap is nil, that implies that the value is true for all elements

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

out has already been allocated here. If an ArrayData implementation lacks buffer slot idx, this access panics; the outer recovery returns an error, but the local buffer is unreachable and leaked. A checked-allocator probe retains 64 bytes on this head versus 0 on the base. Please release out on panic/error after allocation and add a checked-allocator regression test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants