apache / apache/arrow-rs

parquet: lazy-initialize skip_buffer in DeltaBitPackDecoder::skip when all miniblocks are bw=0

Open
#9,792 1 comment 0 reactions 1 assignee Claimed by @etseidl View on GitHub
Dominant language
Rust
Stars
3.6k
Forks
1.3k
Avg merge
2d 18h
Merged PRs (30d)
169

Description

Follow-up to #9786.

`DeltaBitPackDecoder::skip` allocates a `Vec` of `mini_block_batch_size` elements (32 for INT32, 64 for INT64) unconditionally at the top of the non-terminal skip path:

```rust
let mut skip_buffer = vec![T::T::default(); mini_block_batch_size];
while skip < to_skip {
...
let min_delta = self.min_delta.as_i64()?;
if bit_width == 0 {
// bw=0 fast path — does NOT read skip_buffer
...
} else {
// bw>0 — reads into skip_buffer
let skip_count = self.bit_reader.get_batch(&mut skip_buffer[0..mini_block_to_skip], bit_width);
...
}
...
}
```

After #9786, the bw=0 path no longer touches `skip_buffer`. For columns where every miniblock in the skip range is bw=0 (uniform-step and similar patterns), the allocation is pure waste.

### Proposed change

Lazy-init `skip_buffer` inside the `bw > 0` branch, only when the branch is first entered during a skip call. Either:

1. `Option>` initialized to `None`, materialized on first bw>0 miniblock.
2. `Vec::with_capacity(0)` default, re-grown on first bw>0 miniblock via `resize_with`.

Option 1 is cleaner; option 2 keeps the type Vec throughout.

### Context from #9786 review

Reviewer noted during review of #9786:

> I love moving this inside `bw != 0` branch...saves a memset at least. I wonder if we could lazy initialize `skip_buffer` as well. Probably better as a follow on.

### Expected impact

Saves one heap allocation + 128/256 bytes of zero-init per `skip()` call for columns where the skip range is entirely bw=0. Measurable win on uniform-step columns (timestamps with fixed cadence, run-length-like data, single-value columns). Probably noise-level on mixed-bw columns.

### Where

`parquet/src/encodings/decoding.rs`, inside `impl Decoder for DeltaBitPackDecoder::skip`, around line 877 (as of #9786 merge).

### Not a regression

The allocation has always been there pre-#9786. This just observes that #9786 made it conditionally wasteful.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.