Proper aggregate function detection (even deeper in AST) #581
Conversation
This file contains 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
janedbal
commented
Jun 25, 2024
| use Doctrine\ORM\Query\AST; | ||
| use function is_string; | ||
|
|
||
| class QueryAggregateFunctionDetectorTreeWalker extends Query\TreeWalkerAdapter |
There was a problem hiding this comment.
The AST traversal logic is copied from Doctrine's SqlWalker. But since they do double-dispatch which is not possible here, those places are mostly replaced with generic doWalkNode.
I also prefixed all methods with do so that this works in dbal3 and dbal4 (where TreeWalker interface lost most of its methods).
janedbal
commented
Jun 25, 2024
| $this->constantArray([ | ||
| [ | ||
| new ConstantStringType('many'), | ||
| TypeCombinator::addNull(new ObjectType(Many::class)), |
There was a problem hiding this comment.
This wont be nullable in the old implementation, which is wrong.
|
Thank you! |
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.
Separated from #506.
This is needed for proper nullability detection (when no group by is used, single row is always returned). Previously, it was working only when SUM was in first level in select AST. But it can be anywhere inside the tree, so we need pre-walk the AST to detect it.