Skip to content

perf(parquet): decode BSS FLBA into contiguous storage - #1172

Open
fallintoplace wants to merge 3 commits into
apache:mainfrom
fallintoplace:perf/parquet-bss-flba-contiguous
Open

perf(parquet): decode BSS FLBA into contiguous storage#1172
fallintoplace wants to merge 3 commits into
apache:mainfrom
fallintoplace:perf/parquet-bss-flba-contiguous

Conversation

@fallintoplace

Copy link
Copy Markdown
Contributor

Rationale for this change

BYTE_STREAM_SPLIT decoding for FIXED_LEN_BYTE_ARRAY currently allocates a separate byte slice for every output value when the output has no reusable capacity. A 65,536-value batch therefore performs 65,536 decoder allocations.

What changes are included in this PR?

  • Allocate one contiguous backing block for output values that need storage.
  • Keep reusing output slices that already have enough capacity.
  • Give each decoded value an exact-capacity slice.
  • Only prepare the output prefix that is actually decoded.
  • Add coverage for contiguous storage, mixed reusable output, output lifetime across decoder resets, partial output, and spaced decoding.
  • Add cold-output benchmarks for widths 2, 4, 8, 16, and 32.

Apple M1 Pro, GOMAXPROCS=1, 65,536 values:

Width Before After Change Allocations
16 2.42 ms 1.55 ms -35.7% 65,537 to 2
32 3.82 ms 2.61 ms -31.7% 65,537 to 2

Bytes allocated stay unchanged. The existing reusable-output benchmark also remains allocation-free and improved by about 8%.

Are these changes tested?

  • PARQUET_TEST_DATA=$PWD/parquet-testing/data go test ./parquet/...
  • go test -race ./parquet/internal/encoding
  • go vet -composites=false ./parquet/internal/encoding
  • Cross-compiled the encoding package tests for linux/amd64 and linux/s390x.

Are there any user-facing changes?

No.

@zeroshade

Copy link
Copy Markdown
Member

Since this is a draft, i'll hold off on further review until it's marked ready

@fallintoplace
fallintoplace marked this pull request as ready for review August 17, 2026 21:08

@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 contiguous cold-output allocation is correct and substantially reduces allocations when every output entry is empty. Ownership across decoder resets, partial decoding, and spaced decoding also looks sound.

However, the current loop regresses the existing all-reusable steady-state path by about 10–11% on arm64 and can overallocate dramatically for mixed reusable output. I included measurements and a suggested fast-path/slow-path structure inline.

Targeted encoding and race suites pass. File and pqarrow integration tests were blocked only by the unavailable PARQUET_TEST_DATA checkout. GitHub currently reports no CI checks for this head.


This review was drafted by an AI-assisted tool and
confirmed by an Apache Arrow Go maintainer. After you've
addressed the point above and pushed an update, an Apache Arrow Go
maintainer — a real person — will take the next look
at the PR. The findings cite the project's review criteria;
if you think one is mis-applied, please reply on the
PR and a maintainer will weigh in.

More on how Apache Arrow Go handles maintainer review:
CONTRIBUTING.md.

for idx := range output {
if cap(output[idx]) < dec.typeLen {
if storage == nil {
storage = make([]byte, (len(output)-idx)*dec.typeLen)

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.

This sizes the backing block from all remaining outputs rather than from the outputs that actually need storage. In a 65,536-value, width-16 batch where only out[0] lacks capacity, the merge base allocates 16 B while this code allocates 1,048,576 B.

The additional storage state in this loop also regresses the existing all-reusable steady-state benchmark on arm64:

  • 1,024 values: approximately 1,060 → 1,174 ns/op
  • 65,536 values: approximately 69.0 → 76.2 µs/op

Both results were stable across six 500 ms runs with GOMAXPROCS=1.

Since preserving reusable caller storage is part of this optimization, could the existing fast loop remain unchanged until the first insufficient-capacity entry, then delegate the suffix to a cold helper that counts missing entries and allocates exactly missing * typeLen bytes? Please also add a mixed-reuse benchmark or allocation assertion so this case remains covered.

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