alamb commented on PR #15591: URL: https://github.com/apache/datafusion/pull/15591#issuecomment-5424238812
> > my implementation ditched the emit to first and emit all and instead only have emit next block, since it will simplify implementations to not have enums and a lot of generics like this PR have to support both > > The problem with that is that this is breaking change that cannot be done incrementally, like, all the helpers can be created in different pr, but changing to blocked implementation cannot be done incremently. > > Based on my previous experiments, I agree with using only `EmitTo::NextBlock`, and I also think it would be challenging to roll this out incrementally. > > I can participate asynchronously by reviewing PoC PRs. Given the complexity of the implementation, I think we’ll need to look at the actual code to make meaningful progress. Maybe we can add an `EmitTo::NextBlock` to the enum (and left the others), but made all existing aggregators / group key storage error with unimplemented at first? And then we slowly migrated things over? I realize it will be hard to do this incrementally, but I think it would be easier than trying to do it all at once What I suggest is: 1. We push to finish the existing grouping refactor (so we can delete the old hash stream code and focus on one copy at a time) 2. In parallel, prototype what an EmitTo::NextBlock would look like using the new hash aggregate streams (e.g. https://github.com/apache/datafusion/blob/9a3d35351aaa91a4d22d6cb9dc4ce61c76059dc2/datafusion/physical-plan/src/aggregates/mod.rs#L1233-L1279) Maybe we could focus on just one stream type (e.g. `PartialHashAggregateStream` or `FinalHashAggregateStream`) and figure out how EmitTo::NextBloc could work there -- 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]
