Skip to content

perf(arrow/array): append dictionary indices directly - #1313

Open
fallintoplace wants to merge 4 commits into
apache:mainfrom
fallintoplace:perf/arrow-dictionary-append-indices
Open

fallintoplace wants to merge 4 commits into
apache:mainfrom
fallintoplace:perf/arrow-dictionary-append-indices

Conversation

@fallintoplace

Copy link
Copy Markdown
Contributor

What does this change?

  • DictionaryBuilder.AppendIndices used to allocate a new typed slice on every call.
  • Convert the input values directly into the final typed index buffer.
  • Keep the existing bulk validity bitmap path.
  • Add coverage for all eight index types, nullable appends, split appends, and invalid validity lengths.
  • No public API changes.

Benchmark

Command:

go test ./arrow/array -run '^$' -bench '^BenchmarkDictionaryBuilderAppendIndices$' -benchmem -benchtime=300ms -count=8

Apple M1 Pro, Go 1.26.3. The benchmark uses 65,536 indices and a 256-entry dictionary.

Case Before ns/op After ns/op Before B/op After B/op Allocs
int32, non-null 110,370 75,398 815,908 553,762 18 -> 17
int32, nullable 116,578 75,895 815,907 553,762 18 -> 17

This is about 32% less time and allocation bytes for the non-null case, and about 35% less time for the nullable case.

The Parquet decoder's separate []uint64 -> []int scratch conversion is intentionally outside this PR.

Tests

  • go test ./...
  • go test -race ./arrow/array
  • go vet ./arrow/array

@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 direct writes preserve the existing conversions and validity handling across the index builders. Dictionary tests passed (go test -p 2 -count=1 ./arrow/array -run Dictionary). One non-blocking benchmark note below.

func BenchmarkDictionaryBuilderAppendIndices(b *testing.B) {
const (
length = 1 << 16
cardinality = 1 << 8

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.

Non-blocking: the shared workload uses 256 distinct indices, but the int8 case cannot represent indices 128–255; they narrow to negative dictionary indices. Could we cap the shared cardinality at 128, or choose a valid cardinality per index type, so each benchmark materializes a valid dictionary array?

@zeroshade

Copy link
Copy Markdown
Member

Generally looks good but needs to have the lint issue fixed

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