alamb opened a new issue, #23634: URL: https://github.com/apache/datafusion/issues/23634
### Is your feature request related to a problem or challenge? Follow-up to the discussion on #23548 (see https://github.com/apache/datafusion/pull/23548#issuecomment-4990958946). #23548 fixes correctness bugs by making `ReplaceDistinctWithAggregate` keep a `DISTINCT` unless the input's unique key is 1. still unique after joins (`Dependency::Single`), and 2. not a *nullable* unique key -- SQL `UNIQUE` permits multiple NULL keys (NULLs compare distinct), which `DISTINCT` must still collapse. However, `aggregate_functional_dependencies` marks the functional dependency derived from the GROUP BY keys as `nullable` whenever any group key field is nullable: https://github.com/apache/datafusion/blob/a287c0a959310a6a52cafaf9492adc65d0755037/datafusion/common/src/functional_dependencies.rs#L489-L507 This is overly conservative. The `nullable` flag on a `FunctionalDependence` encodes "multiple NULL keys may coexist" (the SQL `UNIQUE` semantic). That can never happen in aggregate output: GROUP BY treats NULLs as equal, so every key combination -- NULL included -- occurs in exactly one output row, like a primary key. As a result, combined with the (correct) guard from #23548, the redundant-dedup elimination for INTERSECT that #22903 enabled regresses. #23548 restores the pre-#22903 expected plan for the `intersect` test in `datafusion/optimizer/tests/optimizer_integration.rs`: ```sql SELECT col_int32, col_utf8 FROM test INTERSECT SELECT col_int32, col_utf8 FROM test INTERSECT SELECT col_int32, col_utf8 FROM test ``` ```text -- with #23548 (guard blocks the dedup elimination; middle Aggregate is redundant) LeftSemi Join: left.col_int32 = test.col_int32, left.col_utf8 = test.col_utf8 Aggregate: groupBy=[[left.col_int32, left.col_utf8]], aggr=[[]] LeftSemi Join: left.col_int32 = right.col_int32, left.col_utf8 = right.col_utf8 Aggregate: groupBy=[[left.col_int32, left.col_utf8]], aggr=[[]] SubqueryAlias: left TableScan: test projection=[col_int32, col_utf8] SubqueryAlias: right TableScan: test projection=[col_int32, col_utf8] TableScan: test projection=[col_int32, col_utf8] -- the plan #22903 produces (the middle Aggregate is eliminated; the semi join -- preserves the distinctness of its aggregated left input) LeftSemi Join: left.col_int32 = test.col_int32, left.col_utf8 = test.col_utf8 LeftSemi Join: left.col_int32 = right.col_int32, left.col_utf8 = right.col_utf8 Aggregate: groupBy=[[left.col_int32, left.col_utf8]], aggr=[[]] SubqueryAlias: left TableScan: test projection=[col_int32, col_utf8] SubqueryAlias: right TableScan: test projection=[col_int32, col_utf8] TableScan: test projection=[col_int32, col_utf8] ``` ### Describe the solution you'd like Create the whole-group-key functional dependency in `aggregate_functional_dependencies` with `nullable = false`, so that GROUP BY output keys are tracked with primary-key-like semantics. This makes the #23548 guard pass for aggregate-derived keys and restores the #22903 plan. I verified locally that with this change on top of #23548, the `intersect` test produces the single-aggregate plan again and the optimizer / sqllogictest suites pass (including the new tests added by #23548: the join-downgrade case is still caught by the `Dependency::Multi` check, and the nullable `UNIQUE` constraint case is still caught because constraint-derived dependencies keep `nullable = true`). The other consumers of the flag remain sound: `downgrade_dependencies` treats aggregate-derived keys the way it already treats primary keys (downgrade to a nullable unique key under outer joins), and `Filter::is_scalar` correctly becomes less conservative for aggregate inputs. ### Describe alternatives you've considered Splitting the flag into "the key may contain NULLs" and "the same NULL key may repeat". Not currently needed: all consumers interpret `nullable` as the latter, so documenting that meaning (and setting the flag accordingly for aggregates) is sufficient. ### Additional context - Suggested by @simonvandel in https://github.com/apache/datafusion/pull/23548#issuecomment-4990958946 - Related: #23548, #22903, #23626 -- 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]
