zeroshade opened a new pull request, #919:
URL: https://github.com/apache/arrow-go/pull/919
### 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
