Rachelint commented on code in PR #11943:
URL: https://github.com/apache/datafusion/pull/11943#discussion_r1716675863
##########
datafusion/expr-common/src/groups_accumulator.rs:
##########
@@ -31,6 +31,13 @@ pub enum EmitTo {
/// For example, if `n=10`, group_index `0, 1, ... 9` are emitted
/// and group indexes '`10, 11, 12, ...` become `0, 1, 2, ...`.
First(usize),
+ /// Emit all groups managed by blocks
+ AllBlocks,
+ /// Emit only the first `n` group blocks,
+ /// similar as `First`, but used in blocked `GroupValues` and
`GroupAccumulator`.
+ ///
+ /// For example, `n=3`, `block size=4`, finally 12 groups will be returned.
+ FirstBlocks(usize),
Review Comment:
It may be a great idea for reducing code changes(we dont need to refactor
the returned emit value for T to Vec<T>).
Actually I found shifting the existing values down is only necessary for
streaming aggr, because it need to record the current sort idx.
But with `Emit::TakeBlock(idx)`, seems we need to record the current block
id in outer, maybe a bit complicated?
we just define the `Emit::NextBlock`, and use the iterator approach to impl
`AllBlocks` and `FirstBlocks` defined now?
If this makes sense, I can try switch the sketch to this way.
```
pub enum EmitTo {
/// Same
All,
/// Same
First(usize),
/// Return the current block of rows from this accumulator
/// We should always use this emit mode in blocked mode accumulator.
CurrentBlock,
}
```
--
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: github-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
