neilconway commented on PR #23636: URL: https://github.com/apache/datafusion/pull/23636#issuecomment-5050236200
Claude found two regressions introduced by this PR: > P0 — datafusion/common/src/functional_dependencies.rs:504 — the nullable = false change introduces a confirmed wrong-results regression via outer joins + nullable-blind FD consumers. > > Chain: the GROUP BY-derived dependency is now non-nullable, so downgrade_dependencies (functional_dependencies.rs:386-390) no longer deletes it when the aggregate sits on the NULL-padded side of an outer join — it survives as nullable=true, Multi. That label is correct under the new semantics, but get_required_group_by_exprs_indices (line 568-585, used by optimize_projections) and get_required_sort_exprs_indices (line 624-630, used by eliminate_duplicated_expr) ignore nullable entirely and prune dependent columns from GROUP BY / ORDER BY. After NULL-padding, a real NULL-key group row and padded all-NULL rows disagree on dependent columns, so the pruning is unsound. I verified with a controlled experiment (this branch vs. merge base, no constraints involved — plain SQL): > > CREATE TABLE t (x INT) AS VALUES (NULL), (NULL); > CREATE TABLE a (z INT) AS VALUES (0), (2); > SELECT g.x, count(*) AS c > FROM a LEFT JOIN (SELECT x, count(*) AS cnt FROM t GROUP BY x) g ON a.z = g.cnt > GROUP BY g.x, g.cnt ORDER BY c; > -- merge base: NULL 1 / NULL 1 (correct), plan groups by [g.x, g.cnt] > -- this PR: NULL 2 (wrong), plan groups by [g.x] — g.cnt was pruned > > The ORDER BY variant also loses its g.cnt tie-breaker in the plan (nondeterministic output order). A third member of the same family, traced but not executed: add_group_by_exprs_from_dependencies (builder.rs:1859-1882) will now accept SELECT g.x, g.cnt … GROUP BY g.x over such a join (previously a planning error) and can emit multiple rows per g.x. > > Suggestion: make the two pruning helpers nullable-aware in this PR (or land that fix first) — skip any dependency where dep.nullable and a source field is nullable, i.e. the exact guard this PR adds to ReplaceDistinctWithAggregate and that Filter::is_scalar (plan.rs:2706-2715) already uses. This is the #21715 follow-up I offered in the PR thread; the point of flagging it here is that this PR converts those latent bugs into live regressions, so they should not be decoupled. > > P1 — datafusion/common/src/functional_dependencies.rs:489-495 — new plan regression: redundant double Aggregate for DISTINCT over GROUP BY output. > > Confirmed empirically: SELECT DISTINCT x FROM (SELECT x FROM tu GROUP BY x) (with tu.x a nullable UNIQUE column) removes the DISTINCT on the merge base but plans a second, redundant Aggregate on this branch. Cause: the first loop propagates the input's UNIQUE dependency with nullable=true onto the aggregate output, and the "GROUP BY expressions do not already act as a determinant" check then suppresses adding the new nullable=false dependency — a weaker fact suppresses a strictly stronger one, and the new nullable-aware consumer then declines to remove the DISTINCT. Suggestion: in the skip-check, only skip if the existing covering dependency is itself non-nullable (or upgrade a propagated dependency to nullable=false when its sources cover the full GROUP BY key set). -- 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]
