Skip to content

fix(parquet): clamp repLevels in byte-array DataPageV2 row-boundary alignment#919

Open
zeroshade wants to merge 1 commit into
apache:mainfrom
zeroshade:fix-parquet-flba-bytearray-v2-rowboundary-clamp
Open

fix(parquet): clamp repLevels in byte-array DataPageV2 row-boundary alignment#919
zeroshade wants to merge 1 commit into
apache:mainfrom
zeroshade:fix-parquet-flba-bytearray-v2-rowboundary-clamp

Conversation

@zeroshade

Copy link
Copy Markdown
Member

Rationale for this change

The ByteArray and FixedLenByteArray column writers run their own adaptive
batching loop in WriteBatch and WriteBatchSpacedWithError. #883 added
DataPageV2 row-boundary alignment to those loops by calling
alignBatchToRowBoundary(repLevels, levelOffset, batch), but passed the caller's
full repLevels slice. The write length n in those loops is derived from
defLevels/values, not repLevels, and nothing requires
len(repLevels) == n.

columnWriter.doBatches (used by the numeric and boolean paths) already clamps
repLevels = repLevels[:total] for exactly this reason — its comment notes "a
caller passing a repLevels slice longer than total must not spill the extra
levels into the column."
The byte-array paths did not.

When a caller passes a repLevels slice longer than n,
alignBatchToRowBoundary (which grows/inspects using len(repLevels)) can grow
the final batch of a wide repeated row past n. The writer then slices
defLevels/values out of range — recovered as an opaque error — or, with
oversized backing buffers, spills extra levels/values into the column.
len(repLevels) == n is safe (the helper returns early at the tail); the bug
requires len(repLevels) > n, which is reachable through the low-level
typed-writer API for repeated columns under DataPageV2.

What changes are included in this PR?

  • In column_writer_types.gen.go.tmpl, both {{if .isByteArray}} blocks
    (WriteBatch and WriteBatchSpacedWithError) now clamp repLevels to n
    before the batching loop, mirroring the guards already in
    columnWriter.doBatches: length check, empty-input short-circuit,
    repLevels = repLevels[:n], and a repLevels[0] == 0 row-boundary start check.
  • Regenerated column_writer_types.gen.go (4 sites: ByteArray and
    FixedLenByteArray × WriteBatch/WriteBatchSpacedWithError).

Are these changes tested?

Yes. TestWriteBatchByteArrayV2OversizedRepLevels writes a repeated ByteArray
and FixedLenByteArray column under DataPageV2 (dictionary disabled, small batch
size) through both WriteBatch and WriteBatchSpacedWithError, passing a
repLevels slice longer than len(defLevels) with a wide final row. It asserts
the write does not error, exactly the requested values are written (no spill),
and the values round-trip. Each of the four cases fails without the fix
(slice bounds out of range [:7] with capacity 6) and passes with it.
./parquet/file/... and ./parquet/pqarrow/... pass (PARQUET_TEST_DATA set).

Are there any user-facing changes?

No API changes. Repeated BYTE_ARRAY/FIXED_LEN_BYTE_ARRAY columns written
under DataPageV2 with a repLevels slice longer than the value/def-level count
no longer risk an out-of-range write error or spilled levels; the byte-array
paths now match the numeric/boolean paths.

…lignment

The ByteArray and FixedLenByteArray column writers run their own adaptive batching loop in WriteBatch and WriteBatchSpacedWithError. apache#883 added DataPageV2 row-boundary alignment there by calling alignBatchToRowBoundary with the caller's repLevels slice, but did not clamp it to the number of levels being written (n is derived from defLevels/values, not repLevels).

columnWriter.doBatches, used by the numeric and boolean paths, already clamps repLevels to the write length for exactly this reason. When a caller passes repLevels longer than n, the byte-array paths could grow the final batch of a wide repeated row past n, slicing defLevels/values out of range (recovered as an opaque error) or spilling extra levels into the column.

Clamp repLevels to n in both byte-array batching paths, mirroring the guards in columnWriter.doBatches (length check, empty-input short-circuit, clamp, row-boundary start check). The change is made in column_writer_types.gen.go.tmpl and regenerated.

Adds TestWriteBatchByteArrayV2OversizedRepLevels covering WriteBatch and WriteBatchSpacedWithError for both ByteArray and FixedLenByteArray with an oversized repLevels slice; each case fails without the fix (slice bounds out of range) and passes with it.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes a DataPageV2 row-boundary alignment bug specific to BYTE_ARRAY and FIXED_LEN_BYTE_ARRAY column writers when callers pass a repLevels slice longer than the effective write length (n derived from defLevels/values). It makes the byte-array batching loops clamp repLevels to n before invoking row-boundary alignment, matching the existing safeguards in columnWriter.doBatches used by other types.

Changes:

  • Clamp repLevels to n (and enforce row-boundary start) in the byte-array WriteBatch adaptive batching loop.
  • Apply the same repLevels clamping in the byte-array WriteBatchSpacedWithError adaptive batching loop.
  • Add a regression test covering oversized repLevels for both ByteArray and FixedLenByteArray, for both write paths, under DataPageV2.

Reviewed changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated no comments.

File Description
parquet/file/column_writer_v2_row_boundary_test.go Adds a regression test ensuring oversized repLevels does not cause out-of-range errors or spilled values for byte-array writers under DataPageV2.
parquet/file/column_writer_types.gen.go.tmpl Clamps repLevels to n in the byte-array write loops before row-boundary alignment, mirroring columnWriter.doBatches.
parquet/file/column_writer_types.gen.go Regenerates typed writers to include the new repLevels clamping logic for ByteArray and FixedLenByteArray.
Files not reviewed (1)
  • parquet/file/column_writer_types.gen.go: Generated file

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

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.

3 participants