CT-1922: Parsing and postgres implementations for model level constraints #7230

peterallenwebb · 2023-03-27T21:27:07Z

resolves #6754

Description

Adds code to parse and validate constraints on models. Implements default (and postgres) adapter support for model level constraint rendering.

Checklist

I have read the contributing guide and understand what's expected of me
I have signed the CLA
I have run this code in development and it appears to resolve the stated issue
This PR includes tests, or tests are not required/relevant for this PR
I have opened an issue to add/update docs, or docs changes are not required/relevant for this PR
I have run changie new to create a changelog entry

github-actions · 2023-03-27T21:27:27Z

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide.

peterallenwebb · 2023-03-29T14:40:41Z

core/dbt/contracts/graph/nodes.py

+
+@dataclass
+class ModelLevelConstraint(ColumnLevelConstraint):
+    pass  # REVIEW: The columns property above should be on this class instead


For reasons I am still working to understand, moving the columns property above to this class, where it would actually makes sense, results in the columns property being missing when the raw dictionary is passed to the adapter implementation of render_raw_model_constraint().

@gerda It seems like there is a mechanism at work which adds defaults values to and removes unexpected values from the raw dictionaries, but is confused about which type this one has. Can you help me understand what's going on here?

"constraints" is not processed as a config, it's a node-level field. So you need to actually copy it at the top of the "patch" method on parsed node, where "description" and "columns" and "access" are copied. It's only on model nodes so you'd also want to check the node.resource_type. (At some point we may want to split out more finely grained "patch" methods...)

I couldn't find the answer, but I want to make sure I actually understand the question. It sounds like if columns is a key in raw_constraint, then BaseAdapter._parse_model_constraint(raw_constraint) does not have a columns attribute, but BaseAdapter._parse_column_constraint(raw_constraint) does have a columns attribute. Is that right?

Is raw_constraint["columns"] just a List[str]? Or is it more complicated?

(At some point we may want to split out more finely grained "patch" methods...)

Thinking along the same lines for patching other new node-level fields that are only on model nodes in the model version parsing spike (version, is_latest_version).

This turned out to be the result of a typo fixed in my last commit.

I can't count the time I've lost to typos. Turns out the computer is right again.

core/dbt/include/global_project/macros/materializations/models/table/create_table_as.sql

mikealfare · 2023-03-29T15:37:23Z

core/dbt/contracts/graph/nodes.py

+
+@dataclass
+class ModelLevelConstraint(ColumnLevelConstraint):
+    pass  # REVIEW: The columns property above should be on this class instead


I couldn't find the answer, but I want to make sure I actually understand the question. It sounds like if columns is a key in raw_constraint, then BaseAdapter._parse_model_constraint(raw_constraint) does not have a columns attribute, but BaseAdapter._parse_column_constraint(raw_constraint) does have a columns attribute. Is that right?

Is raw_constraint["columns"] just a List[str]? Or is it more complicated?

gshank · 2023-03-29T16:42:15Z

core/dbt/parser/schemas.py

-            raise ParsingError(
-                f"Original File Path: ({original_file_path})\nConstraints must be defined in a `yml` schema configuration file like `schema.yml`.\nOnly the SQL table and view materializations are supported for constraints. \n`data_type` values must be defined for all columns and NOT be null or blank.{self.convert_errors_to_string(error_messages)}"
+
+            if isinstance(node, ModelNode) and node.config.contract is True:


This code works, but right now we're doing node-level field setting in the patch method so ideally this should happen in that method if possible. I presume that access to "validate_constraints" is the issue? Can that be changed into a function that can be called in both places?

Alternatively, we could pull that "patch" method into where this code is called from. It doesn't need to stay on the node object...

Per huddle convo, I've added some comments for now and we'll plan to make this more consistent soon.

core/dbt/parser/schemas.py

…el-constraints

emmyoop · 2023-04-06T21:53:37Z

core/dbt/adapters/base/impl.py

+
+    @available
+    @classmethod
+    def render_raw_model_constraints(cls, raw_constraints: Dict[str, Any]) -> str:


Shouldn't the return type be a List here? Probably an Optional[List]?

Suggested change

def render_raw_model_constraints(cls, raw_constraints: Dict[str, Any]) -> str:

def render_raw_model_constraints(cls, raw_constraints: Dict[str, Any]) -> List:

core/dbt/adapters/base/impl.py

VersusFacit · 2023-04-11T05:48:49Z

core/dbt/adapters/base/impl.py

+
+    @classmethod
+    def render_raw_model_constraint(cls, raw_constraint: Dict[str, Any]) -> Optional[str]:
+        constraint = cls._parse_model_constraint(raw_constraint)


Incidentally, I do love the design here since we get to use an enumerated type instead of strings ✨

VersusFacit · 2023-04-11T05:59:14Z

core/dbt/include/global_project/macros/materializations/models/table/columns_spec_ddl.sql

+      {%- for c in constraints -%}
+        {%- set constraint_str = adapter.render_raw_column_constraint(c) -%}
+        {%- if constraint_str -%}
+        {{ ' ' ~ constraint_str }}


VersusFacit · 2023-04-11T06:01:26Z

tests/adapter/dbt/tests/adapter/constraints/test_constraints.py

+        assert len(results) == 1
+        generated_sql = read_file("target", "run", "test", "models", "my_model.sql")
+        generated_sql_list = _normalize_whitespace(generated_sql).split(" ")
+        generated_sql_list = [


nit: This is repeating the same variable name with different semantics and it just strikes me is something we might want to clean up

VersusFacit

Left some syntax-related clean up nits

…el-constraints

CT-1922: Rough in functionality for parsing model level constraints

38bf3ca

cla-bot bot added the cla:yes label Mar 27, 2023

CT-1922: Revise unit tests for new model-level constraints property

b91c361

peterallenwebb changed the title ~~CT-1922: Rough in functionality for parsing model level constraints~~ CT-1922: Parsing and postgres implementations for model level constraints Mar 28, 2023

CT-1922: (Almost) complete support for model level constraints

16a5500

peterallenwebb marked this pull request as ready for review March 29, 2023 14:34

peterallenwebb requested review from a team as code owners March 29, 2023 14:34

peterallenwebb requested review from nssalian and QMalcolm March 29, 2023 14:34

peterallenwebb commented Mar 29, 2023

View reviewed changes

mikealfare reviewed Mar 29, 2023

View reviewed changes

CT-1922: Fix typo affecting correct model constraint parsing.

87731bf

gshank reviewed Mar 29, 2023

View reviewed changes

MichelleArk reviewed Mar 29, 2023

View reviewed changes

core/dbt/parser/schemas.py Outdated Show resolved Hide resolved

peterallenwebb added 2 commits March 29, 2023 13:50

Merge remote-tracking branch 'origin/main' into paw/ct-1922-model-lev…

4f160e6

…el-constraints

CT-1922: Minor code review refinements

1c86e63

gshank approved these changes Mar 29, 2023

View reviewed changes

MichelleArk approved these changes Mar 29, 2023

View reviewed changes

mikealfare approved these changes Mar 30, 2023

View reviewed changes

peterallenwebb added 5 commits March 31, 2023 15:18

CT-1922: Improve whitespace handling

4c34e86

CT-1922: Render raw constraints to constraint list directly

1232180

Merge remote-tracking branch 'origin/main' into paw/ct-1922-model-lev…

14eb429

…el-constraints

CT-1922: Rework base class for model tests for greater simplicity

02cf980

CT-1922: Remove debugging properties. Oops.

d2d0a30

emmyoop reviewed Apr 6, 2023

View reviewed changes

CT-1922: Fix type annotation

fa826e0

VersusFacit reviewed Apr 10, 2023

View reviewed changes

core/dbt/adapters/base/impl.py Show resolved Hide resolved

VersusFacit reviewed Apr 10, 2023

View reviewed changes

core/dbt/adapters/base/impl.py Show resolved Hide resolved

VersusFacit reviewed Apr 11, 2023

View reviewed changes

VersusFacit approved these changes Apr 11, 2023

View reviewed changes

peterallenwebb added 2 commits April 11, 2023 11:17

Merge remote-tracking branch 'origin/main' into paw/ct-1922-model-lev…

f1ccb4b

…el-constraints

Merge remote-tracking branch 'origin/main' into paw/ct-1922-model-lev…

375e80c

…el-constraints

peterallenwebb merged commit 6ac5c90 into main Apr 11, 2023

peterallenwebb deleted the paw/ct-1922-model-level-constraints branch April 11, 2023 17:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CT-1922: Parsing and postgres implementations for model level constraints #7230

CT-1922: Parsing and postgres implementations for model level constraints #7230

peterallenwebb commented Mar 27, 2023 •

edited

Loading

github-actions bot commented Mar 27, 2023

peterallenwebb Mar 29, 2023

gshank Mar 29, 2023

mikealfare Mar 29, 2023

MichelleArk Mar 29, 2023

peterallenwebb Mar 29, 2023

mikealfare Mar 30, 2023

mikealfare Mar 29, 2023

gshank Mar 29, 2023

gshank Mar 29, 2023

peterallenwebb Mar 29, 2023

emmyoop Apr 6, 2023 •

edited

Loading

VersusFacit Apr 11, 2023 •

edited

Loading

VersusFacit Apr 11, 2023

VersusFacit Apr 11, 2023

VersusFacit left a comment

	def render_raw_model_constraints(cls, raw_constraints: Dict[str, Any]) -> str:
	def render_raw_model_constraints(cls, raw_constraints: Dict[str, Any]) -> List:

CT-1922: Parsing and postgres implementations for model level constraints #7230

CT-1922: Parsing and postgres implementations for model level constraints #7230

Conversation

peterallenwebb commented Mar 27, 2023 • edited Loading

Description

Checklist

github-actions bot commented Mar 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emmyoop Apr 6, 2023 • edited Loading

Choose a reason for hiding this comment

VersusFacit Apr 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VersusFacit left a comment

Choose a reason for hiding this comment

peterallenwebb commented Mar 27, 2023 •

edited

Loading

emmyoop Apr 6, 2023 •

edited

Loading

VersusFacit Apr 11, 2023 •

edited

Loading