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.
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
Detect breaking changes to column names and data types in state:modified check #7216
Detect breaking changes to column names and data types in state:modified check #7216
Changes from 1 commit
b66beb6
14a8e30
c5b73ec
2f0d6b7
d3a4cf4
edd9261
b028400
7abbf67
4ee1a12
1cbab4c
95d112c
8540fd3
25be193
9311211
d7ed116
9498809
c711451
8943468
20cd0f3
520fc40
4bf2766
449aa48
8863089
382f66f
4f5a5ae
4b2213a
46ad6d3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This error message looks good to me 👍
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.
If the user is aware they're making a breaking change, and just needs some guidance regarding making a new version of the model - this error message looks good to me too.
That said, I am worried that this error message could be a little opaque for unintented breaking changes, especially as our set of changes are considered breaking changes becomes more nuanced. For example, a user updates a contracted model with constraints from
table
toview
in an attempt to optimize some spend, which raises a breaking change error because the existing constraints can't be validated. Just seeingThere is a breaking change in the model contract
could be confusing, and perhaps the user would actually choose to just revert that change and make it at a later point (bundle it with some other breaking changes). In that scenario, I'd (as an imaginary user) love to see an error message that looks more like:This may not be concern with our current definition of a breaking changes (any change to column name/data_type => breaking change), but given that we're planning to extend this to be more nuanced soon in #7065, I'd leave some design space for it if this scenario seems important to prioritize the UX of (cc @jtcohen6).
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.
Currently the change of a materialization is not a contract change, it's a config change. The ticket doesn't mention materialization -- do you want that to be part of the contract change too?
Creating that more user-friendly error message will probably be about twice as much work than everything else so far. If you want me to spend my time doing that, I can.
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.
Materializations are not part of this issue, but they will be part the work to extend detecting breaking changes in #7065 (which i still blocked by #7067). If we know we'll want more granular error messaging as part of that work, we could set up the foundations here or leave it to #7065.
I agree it's a good deal more work to get that though; we could break off improving the error messaging into a separate issue so it's refined and estimated appropriately.
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.
I added a brief "reason" to the exception message. We can do more elaborate messaging once the other parts are in place.
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.
(good conversation, no notes!)