Repository navigation
perf: avoid flattening and shifting result batches - #577
Conversation
There was a problem hiding this comment.
Verdict: 1 Nit
Looks good, no blocking issues. I compared the new offset-based ResultSlicer with the old splice/flat logic for leftovers, exact fits, fractional limits, NaN, zero and negative limits, undefined limits, unbuffered mode and the concatChunks growth past 2**25, and the results match. One benign difference: concatChunks now keeps holes as undefined where flat() dropped them; no current source produces sparse chunks.
Other findings
- ⚪ Nit — The new CHANGELOG entry has no PR reference. Neighbouring entries end with one (for example
(#471)), so this one should end with(#577).
|
Advancing For slow consumers of wide rows, this keeps substantial processed data alive between fetches. The 2× unread-row-count bound doesn’t bound retained bytes when row sizes vary, and the benchmarks with materialization disabled won’t expose this. Could we preserve the offset optimization while releasing consumed references on each fetch? One option is to copy the unread tail once into owned storage, then clear consumed slots without modifying the source array. |
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
9084370 to
5ea7104
Compare
|
@aakash-saravanan-db PTAL! |
aakash-saravanan-db
left a comment
There was a problem hiding this comment.
LGTM. The updated owned-tail buffer and immediate clearing of consumed slots resolve the row-retention concern without mutating provider batches or previously returned chunks. Chunking, pagination, and the public fetch/iterator/stream flows remain preserved.
Validated on Node 24.22.1: 1,772 unit tests, build, and type checking passed; 2,000 differential scenarios matched main, all 11 offline Arrow CUJ checks passed, and the halfway GC check retained zero consumed objects, matching main.
Before: Accumulate provider batches until the requested chunk size (100,000 rows by default),
splice()the needed prefix from the final batch, save its tail, and join pieces withflat(). Small fetches repeatedly shift the saved tail;fetchAll()also joins withflat().After: Take the needed prefix with
slice()and copy the unread tail once into an owned buffer. Serve leftovers withslice()plus an offset, clearing consumed slots immediately. Compact after more than half the buffer is consumed and reset when empty. This releases returned rows without mutating provider batches or repeatedly shifting the tail. Reuse a single piece or join pieces with a copy loop shared byfetchAll(), capping initial allocation for large joins. Chunk sizes, order, and buffering behavior stay the same.Buffer after each fetch from a 12-row batch, fetching 3 at a time:
·means a cleared slot: the buffer no longer references that row.Local Node 24.14 results against rebased
main(93a9112), with identical locked dependencies. Medians of five sequential fresh-process samples per build after warmup; synthetic providers, no network:fetchAll(), 80M rowsThe review's 10,000-row retention case now keeps zero consumed objects, matching main; the prior PR kept 5,000 at halfway. Retained heap at halfway falls from 20.84 to 0.88 MiB. The owned copy costs ~16 MiB peak RSS in the 1M-row stress case.
Validation: 1,772 unit tests, lint, formatting, type checking, and build pass. Another 2,000 deterministic scenarios match main for rows, source fetches, and
hasMore().