Repository navigation
compute: store temporal bucket updates time-major - #39760
Conversation
Temporal bucket chunks were sorted by data, so every chunk spanned the bucket's times. Splitting a bucket at a time therefore loaded and rewrote every chunk, including spilled ones, and the frontier bucket splits several times per sweep. Store updates as `((time, data), time, diff)` in the existing merge batcher. Each chunk then covers a narrow range of times, and `seal` at the midpoint moves every chunk whose time bounds do not straddle it without loading it. The trailing time stays the batcher's time, so `seal` and its time-bound pass-through work unchanged. Revealed chunks convert back to `(data, time, diff)` columns one at a time. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Co-Authored-By: Claude Opus 5.5 <[email protected]>
ColumnChunker emits one body per input column, of any size. ChunkChunker cuts each body at the batcher's byte bound, so an oversized staging column no longer enters the batcher as one resident chunk. Co-Authored-By: Claude Opus 5.5 <[email protected]>
QA LLM Review1. MEDIUM -- Per-bucket
|
The time-major staging column was a field of every bucket and only ever cleared, so each bucket that received a large push kept a push-sized allocation until it was peeled. That memory sat outside the spill pool and the batcher's size accounting. Build the staging container per push instead and hand it to the chunker, which takes its body out by value. Co-Authored-By: Claude Opus 5.5 <[email protected]>
|
Posted by Claude Code on behalf of @moritz*.* Valid, fixed in 454fd16. The |
I want to shine some light on this. Assuming we store rows in |
frankmcsherry
left a comment
frankmcsherry
left a comment
There was a problem hiding this comment.
As far as I can see this seems fine. The trade-off as I understand it is a potential increase in memory, probably nominal, to reduce the time needed for temporal bucketing extraction. I'm not certain when we engage this mechanism, but probably in the cases where this trade-off is likely to be valuable anyhow. I don't think it's a clean win, but it seems fine.
|
Thanks for the review! |
Posted by Claude Code on behalf of @antiguru.
Temporal bucket chunks were sorted by data, so every chunk spanned the bucket's times. Splitting a bucket at a time therefore loaded and rewrote every chunk, spilled ones included, and the bucket at the frontier splits several times per sweep.
This stores updates as
((time, data), time, diff)in the existing merge batcher. Each chunk then covers a narrow range of times, andsealat the midpoint passes every chunk whose time bounds don't straddle it through without loading it. The trailing time stays the batcher's time, sosealworks unchanged. Revealed chunks convert back to(data, time, diff)columns one at a time. The cost is a second time column per stored record.In a local measurement with 2M records over a 2^20-tick window, swept in 2^12-tick steps with the spill pool enabled, the slowest peel-and-restore step dropped from 65 ms to 8 ms and the whole sweep from 590 ms to 384 ms.
New unit tests
peel_matches_reference_{resident,spilled}compare bucket chain peels against a reference accumulation, with retractions, colliding updates, and updates arriving for buckets that earlier peels split.Independent of #39756, which commits the readied side of these seals. The two conflict textually at the batcher field.
🤖 Generated with Claude Code