Skip to content

perf(parquet): reuse DELTA_BYTE_ARRAY discard storage - #1321

Open
fallintoplace wants to merge 3 commits into
apache:mainfrom
fallintoplace:perf/parquet-delta-byte-array-discard-scratch
Open

fallintoplace wants to merge 3 commits into
apache:mainfrom
fallintoplace:perf/parquet-delta-byte-array-discard-scratch

Conversation

@fallintoplace

Copy link
Copy Markdown
Contributor

What

  • Reuse decoder-owned storage while DELTA_BYTE_ARRAY values are discarded.
  • Add correctness tests and discard benchmarks.

Why

  • Discard currently allocates a new byte slice for every discarded value with a non-empty suffix.
  • Skipping 65,536 values creates 65,535 allocations only to throw the values away.

Implementation

  • Copy the first discarded value into owned scratch because the decoded suffix can alias page data.
  • Grow the scratch buffer amortized and reuse it across pages.
  • Keep empty suffixes on the existing zero-copy path.
  • Benchmark prefix-heavy and low-prefix data at 1,024 and 65,536 values.

The benchmark changes from 65,535 allocations to 0 allocations per operation after warm-up, with about 3.3x lower discard time for 65,536 values.

Tests:

  • go test ./parquet/internal/encoding -count=1
  • go test ./parquet/file -run '^(TestWithEOFReader|TestInvalidHeaders|TestInvalidFooter|TestIncompleteMetadata|TestDeltaLengthByteArrayPackingWithNulls|TestDeltaBinaryPackedMultipleBatches|TestPageStreaming.*|TestPrimitiveReader|TestFullSeekRow|TestSkipEmptyRepeatedRows)$' -count=1
  • go test -race ./parquet/internal/encoding -run 'TestDeltaByteArrayDecoder(DiscardsAllEmptyValues|DiscardCopiesFirstValue|ReusesDiscardScratch|RejectsInvalidPrefixes|KeepsPartialDecodeResults)$' -count=1
  • go vet ./parquet/internal/encoding

@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.

discardScratch can escape through Decode and then be overwritten by a later Discard.

A concrete sequence is values ["aa", "aa", "zz"]: Discard(1) stores "aa" in the scratch; Decode(1) takes the empty-suffix path and returns a prefix backed by that scratch; the next Discard(1) reuses the scratch and changes the already-returned value to "zz".

I reproduced this on the current head: the retained decoded value mutated from "aa" to "zz". This breaks the decoder's existing result-lifetime invariant, including across SetData because the scratch is intentionally retained. Please ensure storage exposed by Decode cannot alias reusable discard scratch and add this Discard→Decode(empty suffix)→Discard transition as a regression case.

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