neilconway commented on code in PR #23636:
URL: https://github.com/apache/datafusion/pull/23636#discussion_r3604891479
##########
datafusion/optimizer/src/replace_distinct_aggregate.rs:
##########
@@ -99,17 +99,31 @@ impl OptimizerRule for ReplaceDistinctWithAggregate {
})));
}
- let field_count = input.schema().fields().len();
- for dep in input.schema().functional_dependencies().iter() {
+ let schema = input.schema();
+ let field_count = schema.fields().len();
+ for dep in 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]
+ // 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.
+ //
+ // The grouping columns must also not contain NULLs because
+ // a nullable UNIQUE constraint permits multiple NULL keys,
+ // but DISTINCT treats NULLs as equal and must still
+ // collapse them.
Review Comment:
This comment seems a bit opaque to me (e.g., I don't think "grouping
columns" is the right phrase). How about:
```
// A nullable dependency allows multiple rows with NULL
in
// their source columns. 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.
```
--
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]
