Adding new constraint logic that will be used with V2 flag #846
Conversation
benc-db
requested review from
andrefurlan-db and
rcypher-databricks
as code owners
|
@alexguo-db since this PR is pure python, I figured it would be a reasonable introduction to dbt PRs. Context is that I'm redoing how we handle constraints from being in SQL templates to being in python. This PR does not include any calls to the new functions; those will come from SQL templates but will be in a separate PR. |
| rendered_constraint = "" | ||
|
|
||
| if constraint.type in render_funcs: | ||
| if constraint.name: |
There was a problem hiding this comment.
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.
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.
| def render_primary_key_for_column(constraint: ColumnLevelConstraint) -> str: | ||
| assert constraint.type == ConstraintType.primary_key | ||
| rendered = "PRIMARY KEY" | ||
| if constraint.expression: |
There was a problem hiding this comment.
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.
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.
| ConstraintType.not_null: ConstraintSupport.ENFORCED, | ||
| ConstraintType.unique: ConstraintSupport.NOT_SUPPORTED, | ||
| ConstraintType.primary_key: ConstraintSupport.NOT_ENFORCED, | ||
| ConstraintType.foreign_key: ConstraintSupport.NOT_ENFORCED, |
There was a problem hiding this comment.
It seems ConstraintType.Custom is also supported?
There was a problem hiding this comment.
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.
| @@ -152,6 +155,23 @@ def information_schema(self, view_name: Optional[str] = None) -> InformationSche | |||
| def StreamingTable(cls) -> str: | |||
| return str(DatabricksRelationType.StreamingTable) | |||
|
|
|||
| def add_constraint(self, constraint: ModelLevelConstraint) -> None: | |||
There was a problem hiding this comment.
Should we check constraint name uniqueness?
There was a problem hiding this comment.
In the adapter we mostly need to validate in cases where the output from Databricks will be confusing, or if it's easy for us to validate with client-side information. Constraint uniqueness is enforced at the schema level I think, so it's probably better to just give the Databricks error back to the user, rather than trying to enforce here, which could require metadata queries.
|
Is there any ability that this will be able to apply constraints without requiring the full contract enforcement? We've been looking to be able to define specific column constraints and PK/FK without having to define the full contract of columns. You'd have to define any column that you want constraints on, but any other columns would be merged in at runtime. |
It's not in this PR, but the V2 materialization approach basically makes a temp view and a final table that the temp view gets merged into. This gives us a number of benefits, including that you don't need to specify contract enforced/column types in your model in order for us to have the capability of adding constraints to individual columns. |
Description
Adds new constraint logic for use with V2 materialization flag. Not currently hooked up to anything (as that will come when I send the PR that splits table materialization into V1 and V2).
Core ideas:
1.) Use constraint config from dbt (as opposed to our homebrew from the 'meta' dictionary)
2.) Process constraints almost entirely in python since they are so logic heavy (this matches what dbt-core is doing anyway)
3.) Mirror but don't depend on dbt built-in functions for accomplishing this (as they expand the surface of adapter, and this logic has no dependency on the Adapter class).
4.) For processing constraint logic, use a functional style since it is pretty much entirely data transformation.
5.) Store processed constraints on Columns or Relations (since Databricks has timing requirements, namely that a.) not null is a column property (not a proper constraint) and b.) that check constraints can only be added via an alter.
6.) Following a functional style, when we 'enrich' an object, return a new copy. This may need to switch to mutable in the future, depending on whether we need these changes cached or not (the relation cache does not have an easy to use update mechanism).
While writing this description, I'm realizing I'm missing unit tests for model constraints, so I will add those prior to merge.
Checklist
CHANGELOG.mdand added information about my change to the "dbt-databricks next" section.