-
Notifications
You must be signed in to change notification settings - Fork 2.2k
dbt Constraints / model contracts #6271
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
Changes from 16 commits
e879cdd
bd1afc7
4b2a881
e8b52e0
01a07d3
3b31a15
1a1f46a
ebaa54c
fd7a47a
7e3a9be
6df02da
bc3c5bc
4b901fd
e29b571
7d085dc
e6559d4
ca89141
2c4a4cf
1975c6b
f088a03
7421caa
a7395bb
5d06524
380bd96
e6e490d
9c498ef
bed1fec
8c466b0
547ad9e
52bd35b
7582531
7291094
b87f57d
5529334
00f12c2
76bf69c
2e51246
5891eb3
5d2867f
801b2fd
5d59cc1
92d2ea7
4f747b0
eba0b6d
ae56da1
de653e4
cfc53b0
fc7230b
e1c72ac
b215b6c
d550de4
9d87463
ce85c96
49a4120
8ffb654
f2f2707
096f3fd
eae4e76
b8c3812
bb1a6c3
b6dbcf6
751cdc8
6bbd797
d364eeb
4a58ece
ac795dd
d452cae
baf18f0
76c0e4f
10ab3cb
6253ed0
307809d
ab4f396
b99e9be
5935201
f59c9dd
7e28a31
3ddd666
fabe2ce
ffec7d7
d338f33
e34c467
44b2f18
17b1f8e
c652367
f9e020d
926e555
c6bd674
bcc35fc
880ed43
426789e
3d61eda
f163b2c
bf45243
dbef42b
c4de8f3
ad07ced
a501c29
59b0298
e46066b
e80b8cd
257eacd
391bd3b
0441417
e0bcb25
dcf7062
903a2cb
c40ee92
111683a
f8b16bc
97f0c6b
2a33baf
6806a7c
1256e7b
0304dbf
b5b1699
d338faa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,6 +17,7 @@ | |
|
|
||
| from dbt.dataclass_schema import dbtClassMixin, ExtensibleDbtClassMixin | ||
|
|
||
| from dbt.exceptions import CompilationException | ||
|
||
| from dbt.clients.system import write_file | ||
| from dbt.contracts.files import FileHash | ||
| from dbt.contracts.graph.unparsed import ( | ||
|
|
@@ -70,6 +71,8 @@ class ColumnInfo(AdditionalPropertiesMixin, ExtensibleDbtClassMixin, Replaceable | |
| description: str = "" | ||
| meta: Dict[str, Any] = field(default_factory=dict) | ||
| data_type: Optional[str] = None | ||
| constraint: Optional[str] = None | ||
| check: Optional[str] = None | ||
| quote: Optional[bool] = None | ||
| tags: List[str] = field(default_factory=list) | ||
| _extra: Dict[str, Any] = field(default_factory=dict) | ||
|
|
@@ -222,7 +225,33 @@ class ParsedNodeDefaults(NodeInfoMixin, ParsedNodeMandatory): | |
| created_at: float = field(default_factory=lambda: time.time()) | ||
| config_call_dict: Dict[str, Any] = field(default_factory=dict) | ||
|
|
||
| def constraints_validator(self): | ||
| materialization_error = {} | ||
| language_error = {} | ||
| data_type_errors = {} | ||
|
|
||
| if self.resource_type == "model" and self.config.constraints_enabled is True: | ||
| if self.config.materialized != "table": | ||
|
||
| materialization_error = {"materialization": self.config.materialized} | ||
|
|
||
| language = str(self.language) | ||
| if language != "sql": | ||
| language_error = {"language": language} | ||
|
|
||
| for column, column_info in self.columns.items(): | ||
| if column_info.data_type is None: | ||
| data_type_error = {column: {"data_type": column_info.data_type}} | ||
| data_type_errors.update(data_type_error) | ||
|
|
||
| if materialization_error or language_error or data_type_errors: | ||
| raise CompilationException( | ||
| f"Only the SQL table materialization is supported for constraints and data_type values must NOT be null or blank.\n Materialization Error: {materialization_error}\n Language Error: {language_error}\n Data Type Errors: {data_type_errors}" | ||
| ) | ||
|
|
||
| # TODO: this is where we see the columninfo object display the data_type and constraint values | ||
| def write_node(self, target_path: str, subdirectory: str, payload: str): | ||
| self.constraints_validator() | ||
|
|
||
| if os.path.basename(self.path) == os.path.basename(self.original_file_path): | ||
| # One-to-one relationship of nodes to files. | ||
| path = self.original_file_path | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -93,6 +93,8 @@ class HasDocs(AdditionalPropertiesMixin, ExtensibleDbtClassMixin, Replaceable): | |
| description: str = "" | ||
| meta: Dict[str, Any] = field(default_factory=dict) | ||
| data_type: Optional[str] = None | ||
| constraint: Optional[str] = None | ||
|
||
| check: Optional[str] = None | ||
|
||
| docs: Docs = field(default_factory=Docs) | ||
| _extra: Dict[str, Any] = field(default_factory=dict) | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1000,6 +1000,12 @@ def warn(msg, node=None): | |
| return "" | ||
|
|
||
|
|
||
| # TODO: remove this later as Sung created this to avoid errors with running dbt-snowflake==1.3.0 | ||
| def warn_or_error(msg, node=None): | ||
|
||
| dbt.events.functions.warn_or_error(GeneralMacroWarning(msg=msg), node=node) | ||
| return "" | ||
|
|
||
|
|
||
| # Update this when a new function should be added to the | ||
| # dbt context's `exceptions` key! | ||
| CONTEXT_EXPORTS = { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -180,6 +180,7 @@ def from_test_block( | |
| ) | ||
|
|
||
|
|
||
| # TODO: create a ConstraintsBuilder equivalent | ||
| class TestBuilder(Generic[Testable]): | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I should NOT need to build a |
||
| """An object to hold assorted test settings and perform basic parsing | ||
|
|
||
|
|
@@ -194,6 +195,7 @@ class TestBuilder(Generic[Testable]): | |
| r"((?P<test_namespace>([a-zA-Z_][0-9a-zA-Z_]*))\.)?" | ||
| r"(?P<test_name>([a-zA-Z_][0-9a-zA-Z_]*))" | ||
| ) | ||
| # TODO: add config args like: "default", "data_type", "constraint", "check" | ||
| # args in the test entry representing test configs | ||
| CONFIG_ARGS = ( | ||
| "severity", | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -177,6 +177,21 @@ def verify_python_model_code(node): | |
| raise ParsingException("No jinja in python model code is allowed", node=node) | ||
|
|
||
|
|
||
| # TODO: Do we need to do this? | ||
| # def verify_model_constraints(node): | ||
|
||
| # try: | ||
| # rendered_model = get_rendered( | ||
| # node.raw_code, | ||
| # {}, | ||
| # node, | ||
| # ) | ||
| # print(rendered_model) | ||
| # if rendered_model != node.raw_code: | ||
| # raise ParsingException("") | ||
| # except (UndefinedMacroException, ParsingException): | ||
| # raise ParsingException("No jinja in python model code is allowed", node=node) | ||
|
|
||
|
|
||
| class ModelParser(SimpleSQLParser[ParsedModelNode]): | ||
| def parse_from_dict(self, dct, validate=True) -> ParsedModelNode: | ||
| if validate: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -132,6 +132,8 @@ def add( | |
| column: Union[HasDocs, UnparsedColumn], | ||
| description: str, | ||
| data_type: Optional[str], | ||
| constraint: Optional[str], | ||
| check: Optional[str], | ||
|
||
| meta: Dict[str, Any], | ||
| ): | ||
| tags: List[str] = [] | ||
|
|
@@ -145,6 +147,8 @@ def add( | |
| name=column.name, | ||
| description=description, | ||
| data_type=data_type, | ||
| constraint=constraint, | ||
| check=check, | ||
|
||
| meta=meta, | ||
| tags=tags, | ||
| quote=quote, | ||
|
|
@@ -157,8 +161,10 @@ def from_target(cls, target: Union[HasColumnDocs, HasColumnTests]) -> "ParserRef | |
| for column in target.columns: | ||
| description = column.description | ||
| data_type = column.data_type | ||
| constraint = column.constraint | ||
| check = column.check | ||
|
||
| meta = column.meta | ||
| refs.add(column, description, data_type, meta) | ||
| refs.add(column, description, data_type, constraint, check, meta) | ||
| return refs | ||
|
|
||
|
|
||
|
|
@@ -168,6 +174,7 @@ def _trimmed(inp: str) -> str: | |
| return inp[:44] + "..." + inp[-3:] | ||
|
|
||
|
|
||
| # TODO: I'll likely need to add a ConstraintsBuilder class similar to TestBuilder within SchemaParser. Test Comment | ||
| class SchemaParser(SimpleParser[GenericTestBlock, ParsedGenericTestNode]): | ||
| def __init__( | ||
| self, | ||
|
|
@@ -310,6 +317,7 @@ def _parse_generic_test( | |
| raise CompilationException(msg) from exc | ||
|
|
||
| original_name = os.path.basename(target.original_file_path) | ||
| # print(f"original_name: {original_name}") | ||
| compiled_path = get_pseudo_test_path(builder.compiled_name, original_name) | ||
|
|
||
| # fqn is the relative path of the yaml file where this generic test is defined, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -585,6 +585,10 @@ | |
| "type": "boolean", | ||
| "default": true | ||
| }, | ||
| "constraints_enabled": { | ||
|
||
| "type": "boolean", | ||
| "default": false | ||
| }, | ||
| "alias": { | ||
| "oneOf": [ | ||
| { | ||
|
|
@@ -830,6 +834,26 @@ | |
| } | ||
| ] | ||
| }, | ||
| "constraint": { | ||
| "oneOf": [ | ||
| { | ||
| "type": "string" | ||
| }, | ||
| { | ||
| "type": "null" | ||
| } | ||
| ] | ||
| }, | ||
| "check": { | ||
| "oneOf": [ | ||
| { | ||
| "type": "string" | ||
| }, | ||
| { | ||
| "type": "null" | ||
| } | ||
| ] | ||
| }, | ||
| "quote": { | ||
| "oneOf": [ | ||
| { | ||
|
|
@@ -849,7 +873,7 @@ | |
| } | ||
| }, | ||
| "additionalProperties": true, | ||
| "description": "ColumnInfo(name: str, description: str = '', meta: Dict[str, Any] = <factory>, data_type: Optional[str] = None, quote: Optional[bool] = None, tags: List[str] = <factory>, _extra: Dict[str, Any] = <factory>)" | ||
| "description": "ColumnInfo(name: str, description: str = '', meta: Dict[str, Any] = <factory>, data_type: Optional[str] = None, constraint: Optional[str] = None, check: Optional[str] = None, quote: Optional[bool] = None, tags: List[str] = <factory>, _extra: Dict[str, Any] = <factory>)" | ||
| }, | ||
| "InjectedCTE": { | ||
| "type": "object", | ||
|
|
||

Uh oh!
There was an error while loading. Please reload this page.
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'm okay with this nomenclature for now. I'll be curious to hear feedback from beta testers.
One thing that's potentially misleading: In addition to enabling
constraints(if they're defined), this also enables (requires) the verification of the number, order, and data types of columns.A long time ago, there was an idea to call this
strict: #1570. (While searching for this issue, I was proud to find that I still know @jwerderits' GitHub handle by memory.)Uh oh!
There was an error while loading. Please reload this page.
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'm open to changing it after community feedback! There are exceptions that I flow through into
DatabaseErrorsvs.ParsingErrorsbecause we get error handling for free(think: data type validation, number of columns mismatch SQL).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.
[For follow-up issues, out of scope for this PR]
After a bit more discussion, I'm thinking about renaming
constraints_enabledto either:stable: true|falsecontracted: true|falseThere 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.
nit: If it has a default value
Falsethen it doesn't need to be anOption type, it could just bebool.It's a shame the python typing lib uses the word
Optionalas a keyword; it's the default value that makes providing the attribute optional when instantiating the class.On another note, a bool with a default value of
Noneforces the use ofOptional[bool]which only makes sense if you must have a lenient (public) API or if there's inheritance compatibility issues.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.
@davehowell Good catch! It looks like @gshank already had the same thought, and implemented this change over in #7002, along with the config rename :)