Skip to content

fix(parquet/compress): allocate uncompressed output for short destinations - #1004

Merged
zeroshade merged 2 commits into
apache:mainfrom
fallintoplace:fix/parquet-uncompressed-nil-dst
Jul 27, 2026
Merged

fix(parquet/compress): allocate uncompressed output for short destinations#1004
zeroshade merged 2 commits into
apache:mainfrom
fallintoplace:fix/parquet-uncompressed-nil-dst

Conversation

@fallintoplace

@fallintoplace fallintoplace commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

The codec contract allows a nil or undersized destination and expects the codec to allocate when needed. The uncompressed codec was just returning the unchanged destination, which could leave callers with empty or truncated output.

This switches Encode, EncodeLevel, and Decode to append-based copying and adds coverage for nil and undersized destinations.

Tests: go test ./parquet/compress

@fallintoplace
fallintoplace requested a review from zeroshade as a code owner July 24, 2026 23:32
@fallintoplace fallintoplace changed the title fix(parquet/compress): allocate uncompressed codec output fix(parquet/compress): grow uncompressed codec destinations Jul 25, 2026
@fallintoplace fallintoplace changed the title fix(parquet/compress): grow uncompressed codec destinations fix(parquet/compress): allocate output for undersized uncompressed dst Jul 25, 2026
@fallintoplace fallintoplace changed the title fix(parquet/compress): allocate output for undersized uncompressed dst fix(parquet/compress): allocate uncompressed output for short destinations Jul 25, 2026

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

Verified against current main: nocodec.Encode/EncodeLevel/Decode do copy(dst, src); return dst, which silently truncates when dst is non-nil but shorter than src — directly contradicting the documented Codec contract that a nil or too-small dst results in a newly allocated slice. Switching to append(dst[:0], src...) is the right fix: it returns full-length output, reuses dst's capacity when it is large enough (so no extra allocation in the common case), and allocates only when required. dst == nil works correctly since nil[:0] is valid.

The internal reader path is unaffected because buf is always sized to lenUncompressed, making the lengths equal. The test genuinely catches the bug — pre-fix, Encode(make([]byte, 1), "arrow") returns just "a". LGTM.

Note: I originally flagged this as stacked on #1003, but I re-verified against current main and the two are now independent — I applied #1003 to a scratch worktree and this still applied cleanly, so no ordering constraint remains.

@zeroshade
zeroshade merged commit 812c829 into apache:main Jul 27, 2026
23 checks passed
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