fix(optimizer): don't merge a derived table with a set-returning projection [CLAUDE] - #8323
Open
ebarkhordar wants to merge 1 commit into
Open
Conversation
…ection The guard in _mergeable only matched exp.Explode, so INLINE and GENERATE_SERIES projections were dropped when the derived table was merged, collapsing the row count. Match exp.UDTF and exp.ExplodingGenerateSeries instead; Explode is a UDTF so it stays covered.
georgesittas
reviewed
Sep 9, 2026
Comment on lines
+78
to
+80
| # An inner projection of one of these types blocks the merge. A set-returning function | ||
| # multiplies the inner rows, so merging drops a projection the outer row count depends on. | ||
| UNMERGABLE_PROJECTIONS = (exp.AggFunc, exp.Select, exp.UDTF, exp.ExplodingGenerateSeries) |
Collaborator
There was a problem hiding this comment.
Why is there a split between this and the constant in pushdown_projections? It seems like the branching is made based on whether we encounter a set-returning function in both (more or less). This is a bit confusing. The fact that we deal with Anonymous differently is also problematic, let's change that to be consistent. The safest of the two is what we already do for pushdown projections, because you can't determine whether an Anonymous function is set-returning.
| # title: derived table with INLINE cannot be merged | ||
| # dialect: spark | ||
| # execute: false | ||
| SELECT t.a FROM (SELECT a, INLINE(b) AS s FROM x) AS t; |
Collaborator
There was a problem hiding this comment.
This is invalid Spark SQL, you need two names for INLINE's alias, since it produces two columns. Let's change the fixture to use INLINE(b) AS (c1, c2) or something.
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
2 participants
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.
Follow-on from #8314. merge_subqueries has its own copy of the set-returning check one rule later, and it still only matches
exp.Explode, so a projection pushdown just kept gets dropped again when the derived table is merged.INLINEgoes the same way now that #8314 made it a UDTF. Widened the guard toexp.UDTFandexp.ExplodingGenerateSeries; Explode is a UDTF so it stays covered. Both cases are in optimizer.sql and fail on main.I left
exp.Anonymousout. Here it would block the merge for every unknown function, which is a lot more expensive than keeping a projection was in pushdown, and it breaks the twoFROM_JSONfixtures. So the STACK case from the #8314 review is still broken. Can add it if you'd rather be conservative, that one seems like your call.make unit is green. Pure Python only, I didn't run make testc.