asolimando commented on code in PR #27113:
URL: https://github.com/apache/datafusion/pull/27113#discussion_r4507894506
##########
datafusion/common/src/functional_dependencies.rs:
##########
@@ -486,42 +478,55 @@ pub fn aggregate_functional_dependencies(
&& source_indices.contains(input_idx)
// A key column can be in the GROUP BY list more than one
// time (`x, x AS k`). Count it one time.
- && !new_source_input_indices.contains(&Some(*input_idx))
+ && !grouped_source_indices.contains(&Some(*input_idx))
{
- new_source_indices.push(idx);
- new_source_input_indices.push(Some(*input_idx));
+ source_output_indices.push(idx);
+ grouped_source_indices.push(Some(*input_idx));
}
}
- let new_target_indices = get_target_indices_from_dependencies(
+ let determined_input_indices =
get_target_indices_from_dependencies(
func_dependencies.iter(),
- &new_source_input_indices,
- );
- let mode = if existing_target_indices == new_target_indices
- && new_target_indices.is_some()
- {
- // If dependency covers all GROUP BY expressions, mode will be
`Single`:
- Dependency::Single
+ &grouped_source_indices,
+ )
+ .unwrap_or_default();
+ // Dependencies are tracked for columns only, so a computed
+ // expression is never counted as determined, even when its inputs
+ // are.
+ let mut determined_output_indices = vec![];
+ for (output_idx, input_idx) in
group_by_input_indices.iter().enumerate() {
+ if let Some(input_idx) = input_idx
+ && determined_input_indices.contains(input_idx)
+ {
+ determined_output_indices.push(output_idx);
+ }
Review Comment:
Thanks, you're right: proving these cases needs no dependencies on
expressions, only the columns each computed expression reads. At first I wanted
to leave this to a follow-up, but I missed that these queries work on `55.2.0`,
so rejecting them is a regression and it is better to support them in this PR.
I will also update the PR description to reflect the latest changes.
Re. the `api change` label: following the [deprecation
`aggregate_functional_dependencies` keeps its signature and is deprecated, and
the new parameter is on
`aggregate_functional_dependencies_with_computed_inputs`. So there is no
breaking change, and the upgrade guide entry describes the deprecation. Let me
know if this still matches your requirements.
I have also added the tests you suggested as they are (8.6 to 8.8).
--
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]
