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
Adding new constraint logic that will be used with V2 flag #846
Adding new constraint logic that will be used with V2 flag #846
Changes from 9 commits
726e6e1
42f973a
2c4a8e4
d60401f
a39798d
e9f7df4
aeca410
30dacc4
86b58d2
275cc1b
819f2ea
4efecf6
b240be4
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.
It seems ConstraintType.Custom is also supported?
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.
Yes, I'm on the fence on that one. I don't think it hurts anything to say that custom is supported but it is not expected by the dbt framework (their code, like mine, just skips validation on custom and doesn't include it in their constraint support map). I think the weird case is that we don't really know whether Databricks is going to support or enforce a custom constraint, because it depends on what the user does with the custom constraint. Custom is basically just a catch-all for if there is some new feature that's not in the adapter yet, the user can manually specify the full expression for it.
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.
Does constraint always have a name? Do we need to validate and throw or skip?
It seems in Databricks we always require the sql to specify CONSTRAINT with a name?
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.
No, if a constraint doesn't have a name then a.) you don't need to specify CONSTRAINT before the constraint definition (that is basically a keyword that tells DBSQL that the next token is going to be a name, rather than a constraint type) and b.) Databricks will generate one for you.
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.
Is expression always exist? If it's empty do we wanna just return PRIMARY KEY without column info?
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.
yes, because this is a column level constraint, so it is perfectly valid to just state that the column is a PRIMARY KEY. In constrast, a model-level constraint needs to know which columns to treat as the primary key. The support for expression is in case users want to add a constraint option such as DEFERRABLE.