neilconway commented on code in PR #23636:
URL: https://github.com/apache/datafusion/pull/23636#discussion_r3632959740
##########
datafusion/optimizer/src/replace_distinct_aggregate.rs:
##########
@@ -99,17 +99,29 @@ impl OptimizerRule for ReplaceDistinctWithAggregate {
})));
}
- let field_count = input.schema().fields().len();
- for dep in input.schema().functional_dependencies().iter() {
- // If the input is already unique on all of its columns
(e.g.
- // it is a GROUP BY over exactly these columns), the
DISTINCT
- // is a no-op and we can simply remove it. The dependency
mode
- // must be `Single`: a `Multi` dependence (e.g. a former
key
- // downgraded by a join) means equal rows may occur
multiple
- // times, so the DISTINCT still has work to do.
- if dep.mode == Dependency::Single
- && dep.source_indices.len() >= field_count
- && dep.source_indices[..field_count]
+ let schema = input.schema();
+ let field_count = schema.fields().len();
+ for dep in schema.functional_dependencies().iter() {
+ // We can remove the DISTINCT entirely if functional
+ // dependencies prove it is already unique and therefore a
+ // DISTINCT would be a no op.
+ let source_indices = &dep.source_indices;
+ let any_source_field_nullable = source_indices
+ .iter()
+ .any(|&idx| schema.field(idx).is_nullable());
+ let nullable = dep.nullable && any_source_field_nullable;
+ if !nullable
+ // The dependency mode must be `Single` because a
`Multi`
+ // dependence means equal rows may occur multiple times
+ && dep.mode == Dependency::Single
+ && source_indices.len() >= field_count
+ // The grouping columns must not contain NULLs because
a
+ // nullable dependency allows multiple rows with NULL
in
+ // the same column (because NULL != NULL). However,
+ // DISTINCT treats NULLs as equal and collapses them,
so
+ // we can only remove the DISTINCT if we know that no
+ // source column can actually be NULL.
Review Comment:
The placement of this comment seems off? It is talking about `nullable`,
which is a few lines up; we should probably separately explain what the
`all(|(idx, f_idx)| idx == *f_idx)` conjunct is for.
--
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]
