fix: do not remove DISTINCT when a unique key was downgraded by a join#23548
Conversation
…a join `ReplaceDistinctWithAggregate` removes `Distinct::All` when a functional dependence covers all input fields, but it ignores the dependency mode: after a join a formerly-unique key is downgraded to `Dependency::Multi`, so the input may still contain duplicates. This SLT test records the current wrong behavior: `SELECT DISTINCT u.id` over a LEFT JOIN returns one row per matching order instead of one row per user.
`ReplaceDistinctWithAggregate` removes `Distinct::All` when a functional
dependence's determinant columns cover all input fields, treating the
input as already deduplicated. However it ignored the dependency mode:
after a join, `FunctionalDependencies::join` downgrades dependencies to
`Dependency::Multi` because a formerly-unique key may now occur in many
rows. Removing the DISTINCT in that case returns duplicate rows.
For example, with `users.id` as a primary key and several orders per
user:
SELECT DISTINCT users.id FROM users LEFT JOIN orders ON users.id = orders.user_id
returned `id` values repeated once per matching order.
Only remove the DISTINCT when the dependency mode is `Single`, i.e. the
determinant key is guaranteed to occur at most once. The sqllogictest
added in the previous commit now shows the correct deduplicated result
and an Aggregate in the plan.
github-actions
Bot
added
optimizer
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
I also made a small proposed PR to fix the nullable issue too:
- simonvandel#1
(merging that will update this PR)
I can file a follow on issue / PR too if you prefer to just merge this PR in as is
| (2, 30); | ||
|
|
||
| query I | ||
| SELECT DISTINCT u.id |
There was a problem hiding this comment.
I verified this fails without the fix
1. query result mismatch:
[SQL] SELECT DISTINCT u.id
FROM users_with_pk u
LEFT JOIN user_orders o ON u.id = o.user_id
ORDER BY u.id;
[Diff] (-expected|+actual)
1
+ 1
2
at .../group_by.slt:5659
2. query result mismatch:
[SQL] EXPLAIN SELECT DISTINCT u.id ...
[Diff] (-expected|+actual)
logical_plan
- 01)Aggregate: groupBy=[[u.id]], aggr=[[]]
- 02)--Projection: u.id
- 03)----Left Join: u.id = o.user_id
...
+ 01)Projection: u.id
+ 02)--Left Join: u.id = o.user_id
...
Error: Execution("1 failures")
| pub enum Dependency { | ||
| Single, // A determinant key may occur only once. | ||
| Multi, // A determinant key may occur multiple times (in multiple rows). | ||
| /// A determinant key may occur only once. |
There was a problem hiding this comment.
this is a nice drive by change
There was a problem hiding this comment.
This fix looks good to me and better than what is on main -- thank you @simonvandel
I filed an issue (well really had claude code do it) for this PR here:
And it also identified another issue
CREATE TABLE t_uniq (x INT UNIQUE) AS VALUES (NULL), (NULL), (1);
SELECT DISTINCT x FROM t_uniq ORDER BY x NULLS LAST;
-- returns: NULL, NULL, 1 (should be: 1, NULL)Which I verified via duckdb:
(venv) andrewlamb@Andrews-MacBook-Pro-3:~/Software/datafusion$ duckdb
DuckDB v1.5.4 (Variegata)
Enter ".help" for usage hints.
memory D create table t (x int unique);
memory D insert into t values (null);
memory D insert into t values (null);
memory D insert into t values (1);
memory D select * from t;
┌───────┐
│ x │
│ int32 │
├───────┤
│ NULL │
│ NULL │
│ 1 │
└───────┘
memory D select distinct x from t;
┌───────┐
│ x │
│ int32 │
├───────┤
│ 1 │
│ NULL │
└───────┘
memory D select distinct x from t order by x nulls last;
┌───────┐
│ x │
│ int32 │
├───────┤
│ 1 │
│ NULL │
└───────┘Here is a proposed fix targeting this branch for your review
|
Thanks @alamb I pushed 9601db8 to restore the snapshot to before #22903. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #23548 +/- ##
=======================================
Coverage ? 80.65%
=======================================
Files ? 1086
Lines ? 366098
Branches ? 366098
=======================================
Hits ? 295271
Misses ? 53233
Partials ? 17594 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
I reverted the changes |
|
I made a PR to improve the docs more: I also made a separate PR to fix the other issue for real |
3 participants
Which issue does this PR close?
Rationale for this change
The
ReplaceDistinctWithAggregateoptimization pass was incorrectly removing deduplication.What changes are included in this PR?
First commit reproduces the bug in an SLT. I opted for SLT instead of a test inside
datafusion/optimizer/src/replace_distinct_aggregate.rssince SLT seems easier to maintain. But let me know if you prefer the test elsewhere.Second commit fixes the bug. Also includes a drive-by documentation change for
Dependencyto surface the field docs.Are these changes tested?
Yes
Are there any user-facing changes?
Yes, query now deduplicates correctly