alamb commented on code in PR #23636:
URL: https://github.com/apache/datafusion/pull/23636#discussion_r3632683119
##########
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:
It might be, but to be honest I find the unit tests for this code to be hard
to understand as it takes so much setup to do anything (builds schemas, calls
with strings, and asserts indices)
I much prefer the .slt end to end cases on the theory that they better cover
what is actually hittable
It woudl be sweet if someone wanted to refactor the dependecy code into
something more readable/easer to work with -- I think its current structure of
a bunch of interrelated free functions is hard to work with
##########
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:
I agree -- I tried to update the comments in
14ae4a8040674dd807ee67b970a434f9c7a80d3b
--
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]
