test: add functional_dependencies.slt covering functional dependency driven optimizations - #23821
Conversation
DataFusion derives functional dependencies from PRIMARY KEY / UNIQUE constraints and from GROUP BY keys, and several optimizer rules consume them. Coverage today is scattered through group_by.slt, which makes it hard to tell which behaviors are correct and which are known bugs. Add a dedicated test file with one section per consumer: 1. ReplaceDistinctWithAggregate (removing DISTINCT) 2. eliminate_duplicated_expr (dropping trailing ORDER BY keys) 3. optimize_projections (dropping GROUP BY expressions) 4. add_group_by_exprs_from_dependencies (selecting non-grouped columns) 5. GROUP BY derived keys on the NULL-padded side of an outer join Each section contrasts a non-nullable PRIMARY KEY against a nullable UNIQUE column, since that is the distinction the consumers get wrong: SQL UNIQUE permits multiple NULL rows, so a UNIQUE column is a key only among the non-NULL rows. The expected results record current behavior. Four cases produce wrong answers today and are labelled BUG with the expected result and a link to the issue tracking it: * 1.2 DISTINCT over a nullable UNIQUE column returns both NULL rows (apache#23634) * 2.2 ORDER BY x, y drops the `y` key, so the NULL rows come back unordered (apache#23818) * 3.2 GROUP BY x, y drops `y`, merging the two NULL groups and losing a row (apache#23819) * 4.2 SELECT x, y ... GROUP BY x returns two rows for the x = NULL group (apache#23820) Only logical plans are shown, since all of these rules run during logical optimization. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
It would be nice to add coverage for multi-column functional dependencies too, but I wanted to keep this PR small to make it eaiser to review |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #23821 +/- ##
==========================================
- Coverage 80.72% 80.71% -0.01%
==========================================
Files 1090 1090
Lines 370384 370384
Branches 370384 370384
==========================================
- Hits 298975 298950 -25
- Misses 53590 53610 +20
- Partials 17819 17824 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Thanks @comphead -- I actually cut down the comments quite a bit from the initial LLM output -- the current set I think explains what is going on. Given these are all bugs related to NULL corner cases (for example, I learned that |
|
|
||
| # These rules all run during logical optimization, so show only logical plans. | ||
| statement ok | ||
| set datafusion.explain.logical_plan_only = true; |
There was a problem hiding this comment.
Only logical plans are shown, since all of these rules run during logical optimization.
Which issue does this PR close?
This PR adds test coverage rather than closing an issue. It documents the current behavior of these bugs so that fixing them shows up as a test change:
Rationale for this change
While reviewing #23636 @neilconway and I kept coming up with more examples of bad plans, and it was not obvious which of the surrounding wrong answers were pre-existing and which the PR introduced.
What changes are included in this PR?
A new
datafusion/sqllogictest/test_files/functional_dependencies.sltwith one section per consumer of functional dependencies:ReplaceDistinctWithAggregate— removingDISTINCTeliminate_duplicated_expr— dropping trailingORDER BYkeysoptimize_projections— droppingGROUP BYexpressionsadd_group_by_exprs_from_dependencies— selecting non-grouped columnsGROUP BYderived keys on the NULL-padded side of an outer joinCases that currently return wrong answers are labelled
BUGwith the expectedresult and a link to the issue:
DISTINCTover a nullableUNIQUEcolumn returns bothNULLrowsORDER BY x, ydrops theykey, so theNULLrows come back unorderedGROUP BY x, ydropsy, merging the twoNULLgroups and losing a rowSELECT x, y ... GROUP BY xreturns two rows for thex = NULLgroupAre these changes tested?
CI
Are there any user-facing changes?
No.