fix: Unnest must not keep its input's uniqueness - #24787
Draft
Dandandan wants to merge 1 commit into
Draft
Conversation
`Unnest::try_new` copied the input's functional dependencies unchanged, with
the comment "We can use the existing functional dependencies". That is not
true for a list unnest, which turns one input row into several. A determinant
that occurred once in the input can occur many times in the output.
Optimizer rules that read those dependencies then produce wrong results. With
a declared key:
CREATE TABLE t_list (k INT, vals INT[], PRIMARY KEY (k))
AS VALUES (1, [10, 20, 30]), (2, [40]);
CREATE TABLE t_join (k INT) AS VALUES (1), (2);
SELECT u.k, u.v FROM (SELECT k, unnest(vals) AS v FROM t_list) u
JOIN t_join j ON u.k = j.k;
returns 2 rows instead of 4, because `eliminate_join` sees `u` as unique on
`k` and rewrites the inner join into a semi join, which drops the repeated
rows. Without the PRIMARY KEY the same query returns 4.
The dependency still holds in the weaker sense: all rows produced from one
input row share the determinant, so it determines the same columns. Downgrade
it to `Dependency::Multi` rather than dropping it, which keeps it useful and
stops it being read as a uniqueness guarantee.
Unnesting a struct produces one row per input row, so those dependencies are
left as they are.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SgqwvctZdvR1ZCz2hbkEJC
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #24787 +/- ##
=======================================
Coverage 81.52% 81.52%
=======================================
Files 1123 1123
Lines 405970 406044 +74
Branches 405970 406044 +74
=======================================
+ Hits 330978 331040 +62
- Misses 55627 55638 +11
- Partials 19365 19366 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Rationale for this change
Unnest::try_newcopies its input's functional dependencies unchanged:That is not true for a list unnest, which turns one input row into several. A
determinant that occurred once in the input can occur many times in the output,
so it is no longer a unique key.
Optimizer rules that read those dependencies then produce wrong results:
returns 2 rows. The correct answer is 4, and removing the
PRIMARY KEYgives4.
eliminate_joinseesuas unique onk, rewrites the inner join into asemi join, and the repeated rows are dropped.
What changes are included in this PR?
The dependency still holds in the weaker sense: every row produced from one
input row carries the same determinant, so it determines the same columns. It
is downgraded to
Dependency::Multirather than dropped, which keeps it usefulfor other purposes and stops it being read as a uniqueness guarantee.
Unnesting a struct produces one row per input row, so those dependencies are
left as they are.
Are these changes tested?
Yes.
functional_dependencies.sltgains the query above, which returns 4 rows,and a plan assertion that a struct unnest still keeps its input's dependencies.
The full sqllogictest suite (504 files) passes.
Are there any user-facing changes?
Queries that unnest a list from a table with a declared key and then join or
aggregate on that key now return correct results.