alamb opened a new pull request, #23636: URL: https://github.com/apache/datafusion/pull/23636
## Which issue does this PR close? - Closes #23634. ## Rationale for this change `aggregate_functional_dependencies` marks the functional dependency derived from the GROUP BY keys as `nullable` whenever any group key field is nullable. That is overly conservative: on a `FunctionalDependence`, `nullable` encodes "multiple NULL keys may coexist" (the SQL `UNIQUE` semantic, where NULLs compare distinct), which 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. With the (correct) guard added in #23548, the too-weak flag blocks the redundant-dedup elimination for INTERSECT that #22903 enabled. See #23634 for the full analysis and plans. ## What changes are included in this PR? `aggregate_functional_dependencies` now creates the whole-group-key dependency with `nullable = false` instead of deriving it from group-key field nullability. The other consumers of the flag remain sound: - `downgrade_dependencies` treats aggregate-derived keys the way it already treats primary keys (downgraded to a nullable unique key under outer joins). - `Filter::is_scalar` correctly becomes less conservative for aggregate inputs (an equality predicate covering the full group key matches at most one row). ## Are these changes tested? Covered by existing tests. On current `main` the `intersect` plan in `datafusion/optimizer/tests/optimizer_integration.rs` is unchanged by this PR (the guard that consults `nullable` in `ReplaceDistinctWithAggregate` is part of #23548). Note the interplay with #23548: once #23548 merges (it restores the pre-#22903 `intersect` snapshot to keep the guard safe), this PR restores the #22903 single-aggregate plan, so the snapshot needs to be updated back as part of rebasing this PR. Verified locally on a branch combining #23548 + this change: the `intersect` test produces the #22903 plan again, and the full optimizer and sqllogictest suites pass -- including the regression tests added by #23548 (the join-downgrade case is still caught by `Dependency::Multi`, and the nullable `UNIQUE` constraint case is still caught because constraint-derived dependencies keep `nullable = true`). ## Are there any user-facing changes? No API changes. Plans over aggregates may improve (e.g. redundant `DISTINCT`/dedup steps above GROUP BY outputs can be eliminated; scalar filters over grouped inputs are recognized as such). -- 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]
