ahlag opened a new pull request, #11907:
URL: https://github.com/apache/arrow-rs/pull/11907
# Which issue does this PR close?
- Closes #11736.
# Rationale for this change
`Decoder::decode` is documented to handle a framed record body that
straddles a chunk boundary: it returns fewer bytes consumed and the caller
passes the rest again with the next chunk. When the split falls after some of
the body has been decoded, those values stay in the field decoders and the body
is then decoded again from its start. Lists and maps come out with duplicated
or shifted items, and other shapes fail at `flush()` with uneven columns. A
split inside a varint fails with `bad varint` instead of waiting for more bytes.
# What changes are included in this PR?
- `Decoder::truncate(rows)` in `reader/record.rs` drops everything after the
first `rows` values, recursing through lists, maps, records, unions and
nullable values. `RecordDecoder::rollback` truncates every field back to the
last complete row.
- `Decoder::decode` calls `rollback` when a framed body is incomplete, and
when an unframed decode fails, so the next call starts from a clean state.
- `AvroCursor` reports a varint cut off by the end of the buffer as `EOF`,
so `is_incomplete_data` treats it as "need more bytes". A malformed varint
(more than 10 bytes) is still a `ParseError`.
- `OffsetBufferBuilder::truncate` (arrow-buffer) and
`PrimitiveBuilder::truncate` (arrow-array), so a rollback costs the same
however many rows the batch already holds. I'm happy to move these to a
separate PR if that's easier to review.
`RecordDecoder::decode` itself is unchanged. An earlier version rolled back
inside it and slowed the decoder benchmarks by 3-16%, since framed decoding
calls it once per record.
# Are these changes tested?
Yes.
- `test_decoder_record_split_across_chunks` encodes single-object streams,
splits each in two at every position, passes the unconsumed bytes again with
the rest, and compares the result with decoding the whole stream at once. It
covers the shapes from the issue (a list or map as the only field, `{id,
tags}`) and a record with nullable, struct, map, decimal, dense union, boolean
and fixed fields, with and without a projection. It fails without the rollback
(wrong rows) and without the varint change (`bad varint`).
- `test_unframed_decode_retry_after_partial_record` retries an unframed
datum after a partial first attempt.
Benchmarks, `decoder` at 10,000 rows, average of the medians from two
alternating runs against main (M-series laptop):
| | main | this PR | change |
|---|---|---|---|
| Int64 | 26.3 µs | 25.7 µs | -2.5% |
| String | 47.1 µs | 46.5 µs | -1.4% |
| Array | 115.0 µs | 114.5 µs | -0.5% |
| Map | 263.9 µs | 260.0 µs | -1.5% |
| Decimal128 | 43.7 µs | 42.3 µs | -3.3% |
| UUID | 68.5 µs | 66.4 µs | -3.1% |
| Enum(Dictionary) | 27.4 µs | 27.4 µs | +0.1% |
| Mixed | 92.0 µs | 90.5 µs | -1.6% |
| Nested(Struct) | 60.1 µs | 60.2 µs | +0.3% |
The `avro_reader` benchmarks are unchanged. Decoding a 200k-row stream in
64-byte chunks takes 63 ns/row with a batch size of 1024 and 66 ns/row with
65536, so the rollback does not grow with the batch.
# Are there any significant user-facing changes?
No. `OffsetBufferBuilder::truncate` and `PrimitiveBuilder::truncate` are new
public methods.
AI use: Claude Code (AI) helped trace the decode path and wrote the first
draft of the fix, the tests and this description; I reviewed the changes,
checked each test fails without the fix, and ran the benchmarks.
--
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]
