neilconway commented on code in PR #23636:
URL: https://github.com/apache/datafusion/pull/23636#discussion_r3604802234
##########
datafusion/common/src/functional_dependencies.rs:
##########
@@ -498,10 +494,15 @@ pub fn aggregate_functional_dependencies(
// GROUP BY expressions come here as a prefix.
item.source_indices.iter().all(|idx| idx < &count)
}) {
- // Add a new functional dependency associated with the whole table:
- // Use nullable property of the GROUP BY expression:
+ // Add a new functional dependency associated with the whole table.
+ //
+ // `nullable` is `false`: a nullable dependence means multiple NULL
+ // keys may coexist (as under a SQL UNIQUE constraint, where NULLs
+ // compare distinct). That cannot happen after grouping: GROUP BY
+ // treats NULLs as equal, so every key combination -- NULL included
+ // -- occurs in exactly one output row, like a primary key.
Review Comment:
Is it possible to write a unit test to check exactly this property?
##########
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.
+ 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
+ && dep.mode == Dependency::Single
+ && source_indices.len() >= field_count
+ && source_indices[..field_count]
Review Comment:
Similarly, I wonder if we can write a unit test to check this property
directly?
##########
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. This prevents removal of
DISTINCT,
// unless 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]
