Zstd: append values in bulk and harden the frame metadata reads - #9099
Conversation
31c8739 to
1c85ef5
Compare
Merging this PR will regress 1 benchmark
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | WallTime | cuda/bitpacked_u8/unpack/3bw[100M] |
299.4 µs | 349.5 µs | -14.35% |
| ⚡ | Simulation | fsl_large |
253.2 µs | 211.2 µs | +19.92% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing rk/zstd-append-to-builder (3d42169) with develop (3d0c124)1
Footnotes
3f1ff67 to
7c295ab
Compare
7c295ab to
ef7a7c1
Compare
| let to = if end == self.n_buffered_values { | ||
| buffer.len() | ||
| } else { | ||
| zstd_value_offset(buffer, from, end - start)? | ||
| }; |
There was a problem hiding this comment.
I think this optimisation hides a bug. Both end and self.n_buffered_values are coming from metadata or validity, so we are returning buffer.len() without looking at the lengths stored in the buffer. If the zstd buffer is corrupt and ends with a length for example, we would silently treat it as a final empty string instead of raising.
So something like 3cat1 in the zstd buffer would add ["cat", ""] to the builder instead of raising
There was a problem hiding this comment.
oof, I think you're right about this
ef7a7c1 to
cf0104e
Compare
cf0104e to
bb07315
Compare
1c7229d to
bf847fd
Compare
`append_to_builder` walked the decompressed frames one value at a time, appending each through a single-value call and interleaving null runs by hand. It now derives the value byte range from the slice's own metadata and hands the whole region to `append_value_slices`, which sizes the offsets, byte storage and validity once — one offset store plus one `memcpy` per value. Adds a `VarBinViewBuilder` path as well: the frames already hold the values contiguously, so the views can reference them in place instead of going through the canonical array and rewriting every view a second time to rebase its buffer index. `reconstruct_views` therefore takes the buffer index the views should start at. Frame metadata comes straight off disk, so the arithmetic it drives is now checked and surfaced as errors rather than panics or wrapped lengths: value counts, frame sizes, length prefixes and the offsets they walk to. The missing `n_values` fallback no longer reads a byte count as a value count for variable-width values, where that mis-attributes values to frames — it is accepted only for the single-frame case that is still recoverable. Decompression also writes through `WriteBuf` into uninitialized spare capacity rather than a `&mut [u8]` over memory nothing has written yet. Signed-off-by: Robert Kruszewski <robert@spiraldb.com>
bf847fd to
62fd68f
Compare
I, Robert Kruszewski <github@robertk.io>, hereby add my Signed-off-by to this commit: 9fdbe75 Signed-off-by: Robert Kruszewski <github@robertk.io>
2 participants
Write zstd values directly into the builder without additional copy