-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
schemadiff: validate views' referenced columns #12147
Closed
shlomi-noach
wants to merge
18
commits into
vitessio:main
from
planetscale:schemadiff-validate-view-columns-deps
Closed
Changes from all commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
c4c213b
validate table columns referenced by views
shlomi-noach 3dac907
validate views' columns
shlomi-noach e88d426
support star expression
shlomi-noach 61de6b0
removed 'TODO'
shlomi-noach 389872c
rename: errs
shlomi-noach 56c8421
use mutierr
shlomi-noach 11d6379
excessive test, removed
shlomi-noach f4fd673
go mod tidy
shlomi-noach a4ae98d
update to latest mutierr
shlomi-noach ef16bdf
merge main, resolve conflict
shlomi-noach 54fb73a
avoid setting entity columns in case of error
shlomi-noach 2d3e70b
grammar
shlomi-noach b69ede6
schemadiff: fix scenario where no tables exist in schema and with jus…
shlomi-noach bc34369
dual, not dual2
shlomi-noach bc8799d
stripped irrelevant comments
shlomi-noach dd1bc82
merge main, resolve conflict
shlomi-noach 7f6fd13
using AllErrorRecorder instead of multierr package
shlomi-noach 4939d01
multierr update
shlomi-noach File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI -
sqlparser.Walk
will only return the error you return from inside the visitor function, and since you don't return any errors from that, no errors will ever make it out.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@systay ah, great! Thank you. I'll keep the check as it is, for safety, but good to know!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@systay Right, but the errors are gathered here in
errs
? And those are at the end returned if the walker itself doesn't error (which is not expected).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dbussink yeah, that was my point. Not really necessary to catch the returned error from
sqlparser.Walk
since that is not how we are dealing with the errors. When I know it will never return an error, I usually write.OTOH - it's probably good defensive programming to do as @shlomi-noach is doing here and catching and checking the error anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@systay Right, guess as someone now knowing the details too much about how
Walk
is implemented, I wouldn't know that it can't return an error and would still write it defensively 😄.