asolimando commented on code in PR #27113:
URL: https://github.com/apache/datafusion/pull/27113#discussion_r4519075078
##########
datafusion/common/src/functional_dependencies.rs:
##########
@@ -532,7 +574,19 @@ pub fn aggregate_functional_dependencies(
// aggregation:
if !group_by_input_indices.is_empty() {
let count = group_by_input_indices.len();
- let source_indices = (0..count).collect::<Vec<_>>();
+ // Remove extraneous columns (the GROUP BY expressions that the others
+ // determine) from the key, so that the key still exists after a
+ // projection removes them.
+ let source_indices = match get_required_group_by_exprs_indices(
+ aggr_input_schema,
+ group_by_input_indices,
+ ) {
+ Some(mut indices) => {
+ indices.sort_unstable();
+ indices
+ }
+ None => (0..count).collect(),
+ };
Review Comment:
Thanks, good catch! I addressed your comments with aef831217, which applies
your change and adds your two tests as 8.6 and 8.7. I checked that 8.6 fails
without the change, and that 8.7 returns no row without the empty-key fallback.
On top of it, I did a pass in 360c07238 for readability: deciding whether a
GROUP BY expression is determined is now done in one place, the new
`group_by_positions_determined_by` helper, which both the dependency loop and
`group_by_key_indices` use. I also gave both helpers a doc with an example, as
the logic is quite complex. The behavior is the same, and the tests are
unchanged. I kept it as a separate commit, so we can easily drop it if you
prefer your version.
--
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]
