alamb commented on code in PR #13463:
URL: https://github.com/apache/datafusion/pull/13463#discussion_r1846868210
##########
datafusion/physical-plan/src/aggregates/row_hash.rs:
##########
@@ -859,14 +859,13 @@ impl GroupedHashAggregateStream {
)?;
}
_ => {
+ if opt_filter.is_some() {
+ return internal_err!("aggregate filter should be
applied in partial stage, there should be no filter in final stage");
Review Comment:
Well the fact that no queries error and all tests pass is certainly
compelling. I am not sure how good our test coverage is in this area, but this
looks good to me
##########
datafusion/functions-aggregate/src/count.rs:
##########
@@ -467,7 +467,7 @@ impl GroupsAccumulator for CountGroupsAccumulator {
&mut self,
values: &[ArrayRef],
group_indices: &[usize],
- opt_filter: Option<&BooleanArray>,
+ _opt_filter: Option<&BooleanArray>,
Review Comment:
I recommend we put a comment here explaining why we are ignoring the filter.
In general if your assumption holds, we could actually remove the
`opt_filter` argument for all calls to `merge_batch` 🤔
--
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]
