alamb commented on code in PR #23489: URL: https://github.com/apache/datafusion/pull/23489#discussion_r3634246904
########## docs/source/library-user-guide/upgrading/55.0.0.md: ########## @@ -276,6 +276,40 @@ it was `None`), that code can simply be deleted. See [issue #22775](https://github.com/apache/datafusion/issues/22775) for details. +### `GroupsAccumulator::convert_to_state` is now required + +`datafusion_expr_common::groups_accumulator::GroupsAccumulator::convert_to_state` +no longer provides a default implementation, and the +`GroupsAccumulator::supports_convert_to_state` capability method has been +removed. All `GroupsAccumulator` implementations must now support converting Review Comment: 👍 ########## datafusion/physical-plan/src/aggregates/mod.rs: ########## @@ -3591,6 +3591,87 @@ mod tests { Ok(()) } + #[tokio::test] + async fn partial_hash_skip_aggregation_uses_required_convert_to_state() -> Result<()> Review Comment: I don't understand what extra coverage this is adding -- if we are testing partial state conversion I think we should write the tests using sql in sqllogictests. This test has a lot of setup and like I mentioned I am not sure about its coverage value -- 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]
