Fix an aggregate over a whole-record argument planning incorrectly - #4501
Open
robert-brunel wants to merge 1 commit into
Open
Fix an aggregate over a whole-record argument planning incorrectly#4501robert-brunel wants to merge 1 commit into
robert-brunel wants to merge 1 commit into
Conversation
* Change **Expression.pullUp()** to simplify _both_ sides before matching them, rather than only the candidate. * Change **OrderByExpression.pullUp()** the same way, which keeps tolerating an ambiguous match as it did before. An aggregate whose argument effectively reconstructs a whole record, such as `SELECT COUNT((T.*)) FROM T`, failed to plan. In a grouped query the same aggregate value occurs twice, in the group-by expression that computes it and in the projection, where `pullUp()` has to replace it with a reference to the group-by output by matching the two structurally. Matching the *unsimplified* projection against the simplified candidate could not succeed for that argument, since the simplification rules canonicalize it: `Values.collapseSimpleSelectMaybe()` collapses `(T.*)` to just `T`. An `ORDER BY` over an aggregate remains unsupported either way; the whole-record spelling is now rejected as such instead of failing at evaluation time. Fixes #4481.
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
1 participant
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.
Fix incorrect plans for aggregates over a whole-record argument
Expression.pullUp()to simplify both sides before matching them, rather than only the candidate.OrderByExpression.pullUp()the same way.An aggregate whose argument effectively reconstructs a whole record, such as
SELECT COUNT((T.*)) FROM T, failed to plan. In a grouped query the same aggregate value occurs twice, in the group-by expression that computes it and in the projection. In the projection,pullUp()has to replace the expression with a reference to the group-by output for the plan to stay correct. This is done by matching the two values structurally. Matching the unsimplified projection against the simplified candidate could not succeed for that argument, since the simplification rules canonicalize(T.*)to justTviaValues.collapseSimpleSelectMaybe().Fixes #4481.