Detect invalid uses of * and window functions in queries. #1710
Conversation
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
These are for semantically invalid queries that should be detected during analysis.
This prevents a dependency cycle.
We need to resolve functions to know whether a * is being used in an aggregate function or not.
|
The new package |
max-hoffman
approved these changes
Apr 12, 2023
There was a problem hiding this comment.
lgtm, we do use sql/errors.go pretty heavily if you wanted to group them there. More testing is usually good, even if just to help future refactors. Segmentation/organization of errors into subpackages probably wouldn't hurt. I'd expect the Dolt auto-bump for this to pass, which you should get notified for after merging.
The docstring for transform.Inspect and sql.Inspect requires that the inputs not be nil. If they are, something has gone wrong and we shouldn't be silently ignoring it. Also removed a call to IsDDLNode that was copied over from validateOperands but doesn't seem to server a purpose.
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.
This PR is the GMS side of the fix for dolthub/dolt#5656.
Preventing panics from invalid window functions is easy: replace the panic with returning a new kind of error.
The invalid *s were trickier. I added an additional analysis rule that runs immediately after resolving function names. It checks for any uses of "*" in a subexpression (so, not just "SELECT *") that aren't used inside a COUNT, COUNTDISTINCT, or JSONARRAY function.
It's possible that there's other places where *s are allowed that we need to account for. It's also possible that there may be some other disallowed uses of * that will pass this and still cause a panic.