rluvaton commented on PR #25877: URL: https://github.com/apache/datafusion/pull/25877#issuecomment-5916913660
> Thanks @rluvaton , I left some suggestions > > * **Test for multi-block spilling.** The table tests use `MIN_BLOCK_SIZE + 10` groups but never spill, and the existing spill tests stay under 2^18 groups per table, so `sort_and_spill_batches` never sees more than one batch. A `SingleHashAggregateStream` (or Final) test with a memory limit and more than `MIN_BLOCK_SIZE` groups, compared against an unlimited run, would cover it. changed to block size without 2^18, so it is now covered > * **Emit modes that can't be reached yet.** `materialize_batches` is only called with `EmitTo::All` (`common.rs:348`, `common.rs:416`), so `BlockedEmitTo::NextBlock`/`First`, `BlockedVec::take_first`, the map-renumbering arms in `blocked/primitive.rs:174-215`, and `FIXED_BLOCK_SIZE` are unused. Would you consider moving them to the ordered-stream PR that needs them? It would make this one easier to review. Fine to leave if you prefer. `FIXED_BLOCK_SIZE` is for nested/dictionary support so I won't have to introduce breaking changes first and next block will be used by ordered but also outside of datafusion so they are valuable from the start -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
