dbt Constraints / model contracts #6271

sungchun12 · 2022-11-16T18:51:23Z

resolves #6079

Description

This is a building block to empower multi-project experiences in the future.

Brings defining data types for columns within a SQL select statement as native to dbt. This PR focuses on injecting new config defaults within the manifest, and introduces compilation defense code to ensure configs follow a clear standard. Other adapter PRs will pair with this one to ensure configs cascade appropriately.

Note: It is the responsibility of the adapter to provide extra guardrails for configs like check that do not work universally across databases.

Related Adapter Pull Requests

Must be reviewed with passing tests

dbt Core Test Cases

Fix failing test regressions (Thanks @MichelleArk for helping me across the finish line 💯 )
Test: Verify compilation errors show expected dynamic messages
Test: Verify config hierarchy overrides work as expected(ex: constraints enabled at the model level override constraints enabled at the project_yml level with True or False values)
Test: Verify only SQL table materializations work
Test: Verify all or nothing data_type configs are enforced

dbt-postgres Adapter Test Cases

Test: Verify global macro is dispatched and can be overridden elegantly in the postgres adapter
Test: Verify DDL matches an expectation
Test: rollback the full DDL transaction if it fails with an ad hoc query verifying the table is in its previous state

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

…bt-constraints

github-actions · 2022-11-16T18:51:41Z

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.

github-actions · 2022-11-16T18:51:41Z

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.

…raints

sungchun12 · 2022-11-17T22:30:43Z

Progress Update 11/17/2022

defaults for data_type, constraint, check appear in the manifest.json
Add a constraints validator within parsed.py before a node can be written to compiled SQL code
only SQL table materializations are allowed
performance is fast

Next Steps

Verify if my approach is an accepted pattern
Write down test cases

sungchun12 · 2022-11-18T21:17:16Z

core/dbt/contracts/graph/parsed.py

@@ -17,6 +17,7 @@

 from dbt.dataclass_schema import dbtClassMixin, ExtensibleDbtClassMixin

+from dbt.exceptions import CompilationException


I noticed no exceptions exist in this file except for the one I introduce. Is that intentional?

sungchun12 · 2022-11-18T21:18:07Z

core/dbt/contracts/graph/unparsed.py

@@ -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


It only needs to be inserted here as external tables and sources are not in scope for this feature

Could there ever be multiple constraints? Should this be a list (or either a string or a list)?

Ooooo great question. For developer UX, I'd bias towards a list even though it may be more "efficient" to allow arbitrary strings, developer mental load better reasons about multiple constraints as a list.

And yes, postgres allows multiple constraints: https://www.postgresql.org/docs/current/ddl-constraints.html#id-1.5.4.6.6

I'll make the change for the constraints config to be an optional list

String Format:
Harder to reason about separate constraints

# models/my_custom_model_config.yml models: - name: my_model columns: - name: email data_type: string constraints: not null unique

List Format:
Easier to reason about separate constraints

# models/my_custom_model_config.yml models: - name: my_model columns: - name: email data_type: string constraints: ['not null','unique']

sungchun12 · 2022-11-18T21:19:55Z

core/dbt/exceptions.py

@@ -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):


Is there a reason this function was removed from the main branch while 1.3.latest still has it? https://github.com/dbt-labs/dbt-core/blob/1.3.latest/core/dbt/exceptions.py#L1090

I also notice that there looks to be a regression error that any selected models give an incorrect warning on the main branch.

Ah, this was related to work that Emily has been doing on more structured logging/exceptions. Just opened a ticket for it: #6297

Looks like the selector error message was fixed, but is there a reason warn_or_error doesn't exist in the main branch but in 1.3.latest? @jtcohen6

@sungchun12 It still exists, it just moved:

- from dbt.exceptions import warn_or_error + from dbt.events.functions import warn_or_error

Ahhhh I know what happened, I needed to update my snowflake adapter to the main branch vs. relying on 1.3.latest Thanks Jerco! I'm unblocked!

sungchun12 · 2022-11-18T21:47:15Z

core/dbt/parser/generic_test_builders.py

@@ -180,6 +180,7 @@ def from_test_block(
        )


+# TODO: create a ConstraintsBuilder equivalent
 class TestBuilder(Generic[Testable]):


I should NOT need to build a ConstraintsBuilder because the configs extend column_info within the schemas.py file.

sungchun12 · 2022-11-18T21:52:52Z

core/dbt/parser/models.py

@@ -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):


We don't need to verify within this model because that happens parsed.py

sungchun12 · 2022-11-18T21:58:02Z

schemas/dbt/manifest/v8.json

@@ -585,6 +585,10 @@
          "type": "boolean",
          "default": true
        },
+        "constraints_enabled": {


How do you test the manifest?

quis verificat ipsos verificantia? quis manifestat ipsos manifesta?

google translate:

Who verifies the verifiers? Who makes them manifest?

jtcohen6 · 2023-01-30T13:55:35Z

I've reviewed + refactored tests on the dbt-core PR, with the specific goal of having a clearer separation between:

What can be tested in dbt-core only: validation, parsing exceptions, basic end-to-end functionality
What needs testing on each adapter: dbt's "preflight" checks within materialization; platform-enforced runtime checks (via DDL); successful rollback if contract isn't met

There are now only two "adapter zone" test classes that need inheriting/modifying. I'll take a pass over the adapter PRs tomorrow or Wednesday.

Ideally, I'd like to get all of this merged by Wednesday (Feb 1), but I'm going to wait until feature/click-cli is merged (#6631), so as to avoid creating additional merge conflicts for that crew.

dave-connors-3 · 2023-01-30T16:37:59Z

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

+  cast('2019-01-01' as date) as date_day
+"""
+
+my_model_error_sql = """


this looks the same as my_model_sql -- what's the expected error? @jtcohen6

@dave-connors-3 Good catch! Ended up implementing this by other means below:

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

Lines 235 to 237 in dcf7062

# Make a contract-breaking change to the model

my_model_with_nulls = my_model_sql.replace("1 as id", "null as id")

write_file(my_model_with_nulls, "models", "my_model.sql")

Let's cut my_model_error_sql entirely

dave-connors-3 · 2023-01-30T17:10:48Z

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

+
+        # Verify the previous table still exists
+        old_model_exists_sql = """
+            select id from dbt.{0}.my_model where id = 1


@jtcohen6 I think this will also need the templated database!

…raints

jtcohen6

Thanks to @dbeatty10 @McKnight-42 @MichelleArk for doing some live PR walkthrough & review earlier!

I'll work on fixing up the merge conflicts. Let's make sure all the TODOs are captured in our tracking issues, then let's get this baby merged 😎

jtcohen6 · 2023-02-14T17:16:35Z

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

+      {%- set col_name = col['name'] -%}
+      {%- set column_names_config_only = column_names_config_only.append(col_name) -%}
+    {%- endfor -%}
+    {%- set sql_file_provided_columns = get_columns_in_query(sql) -%}


Making an assumption here that column ordering is consistent (and deterministic) in all our data platforms.

Follow-on TODO: We prefer option 2 in https://github.com/dbt-labs/dbt-core/pull/6271/files#r1069332715, let's make sure dbt always ensures order itself, and doesn't make yaml-ordering matter so much

Thanks @MichelleArk for opening #6975!

jtcohen6 · 2023-02-14T17:34:00Z

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

+    {% if config.get('constraints_enabled', False) %}
+      {{ get_assert_columns_equivalent(sql) }}
+      {{ get_columns_spec_ddl() }}
+    {% endif %}


Follow-up (related to ongoing discussion in #6750): We might want to distinguish between the "contract" and the "constraints." That distinction would make it possible for users to define (e.g.) check constraints on Postgres/Databricks, without providing the full column specification in a yaml file. (This is already the case in dbt-databricks.)

What ought to be part of the contract (for detecting breaking changes, #6869)? "Data shape" for sure (column names, types, nullability). Additional constraints? Tests? We need to decide

Assigning myself to #6750 for further refinement, before we pick this up for implementation

jtcohen6 · 2023-02-14T17:39:32Z

schemas/dbt/manifest/v8.json

@@ -173,7 +173,7 @@
    }
  },
  "additionalProperties": false,
-  "description": "WritableManifest(metadata: dbt.contracts.graph.manifest.ManifestMetadata, nodes: Mapping[str, Union[dbt.contracts.graph.nodes.AnalysisNode, dbt.contracts.graph.nodes.SingularTestNode, dbt.contracts.graph.nodes.HookNode, dbt.contracts.graph.nodes.ModelNode, dbt.contracts.graph.nodes.RPCNode, dbt.contracts.graph.nodes.SqlNode, dbt.contracts.graph.nodes.GenericTestNode, dbt.contracts.graph.nodes.SnapshotNode, dbt.contracts.graph.nodes.SeedNode]], sources: Mapping[str, dbt.contracts.graph.nodes.SourceDefinition], macros: Mapping[str, dbt.contracts.graph.nodes.Macro], docs: Mapping[str, dbt.contracts.graph.nodes.Documentation], exposures: Mapping[str, dbt.contracts.graph.nodes.Exposure], metrics: Mapping[str, dbt.contracts.graph.nodes.Metric], selectors: Mapping[str, Any], disabled: Optional[Mapping[str, List[Union[dbt.contracts.graph.nodes.AnalysisNode, dbt.contracts.graph.nodes.SingularTestNode, dbt.contracts.graph.nodes.HookNode, dbt.contracts.graph.nodes.ModelNode, dbt.contracts.graph.nodes.RPCNode, dbt.contracts.graph.nodes.SqlNode, dbt.contracts.graph.nodes.GenericTestNode, dbt.contracts.graph.nodes.SnapshotNode, dbt.contracts.graph.nodes.SeedNode, dbt.contracts.graph.nodes.SourceDefinition]]]], parent_map: Optional[Dict[str, List[str]]], child_map: Optional[Dict[str, List[str]]])",
+  "description": "WritableManifest(metadata: dbt.contracts.graph.manifest.ManifestMetadata, nodes: Mapping[str, Union[dbt.contracts.graph.nodes.AnalysisNode, dbt.contracts.graph.nodes.SingularTestNode, dbt.contracts.graph.nodes.HookNode, dbt.contracts.graph.nodes.ModelNode, dbt.contracts.graph.nodes.RPCNode, dbt.contracts.graph.nodes.SqlNode, dbt.contracts.graph.nodes.GenericTestNode, dbt.contracts.graph.nodes.SnapshotNode, dbt.contracts.graph.nodes.SeedNode]], sources: Mapping[str, dbt.contracts.graph.nodes.SourceDefinition], macros: Mapping[str, dbt.contracts.graph.nodes.Macro], docs: Mapping[str, dbt.contracts.graph.nodes.Documentation], exposures: Mapping[str, dbt.contracts.graph.nodes.Exposure], metrics: Mapping[str, dbt.contracts.graph.nodes.Metric], selectors: Mapping[str, Any], disabled: Union[Mapping[str, List[Union[dbt.contracts.graph.nodes.AnalysisNode, dbt.contracts.graph.nodes.SingularTestNode, dbt.contracts.graph.nodes.HookNode, dbt.contracts.graph.nodes.ModelNode, dbt.contracts.graph.nodes.RPCNode, dbt.contracts.graph.nodes.SqlNode, dbt.contracts.graph.nodes.GenericTestNode, dbt.contracts.graph.nodes.SnapshotNode, dbt.contracts.graph.nodes.SeedNode, dbt.contracts.graph.nodes.SourceDefinition]]], NoneType], parent_map: Union[Dict[str, List[str]], NoneType], child_map: Union[Dict[str, List[str]], NoneType])",


TODO before merging: Regenerate this given latest changes.

FOR LATER: Bump the manifest schema version to v9 (for v1.5) — until/unless we decide to change our approach to manifest/artifact versioning :) cc @MichelleArk let's talk more about this with @dbt-labs/core-language

Lots of code written & reviewed since!

sungchun12 · 2023-02-15T16:56:24Z

Thanks team for getting this done. I just shouted out loud, "FINALLY!" With the utmost glee.

sungchun12 added 3 commits November 7, 2022 12:13

start off the blueprints

e879cdd

Merge branch 'main' of https://github.com/dbt-labs/dbt into feature/d…

bd1afc7

…bt-constraints

Merge branch 'main' of https://github.com/dbt-labs/dbt into feature/d…

4b2a881

…bt-constraints

cla-bot bot added the cla:yes label Nov 16, 2022

test commit

e8b52e0

sungchun12 self-assigned this Nov 16, 2022

sungchun12 mentioned this pull request Nov 16, 2022

dbt Constraints #6233

Closed

6 tasks

sungchun12 added 11 commits November 16, 2022 12:37

working snowflake env

01a07d3

update manifest expectation

3b31a15

add error handling

1a1f46a

clean up language

ebaa54c

constraints validator

fd7a47a

cleaner example

7e3a9be

better terminal output

6df02da

add python error handling

bc3c5bc

Merge branch 'main' of https://github.com/dbt-labs/dbt into dbt-const…

4b901fd

…raints

add to manifest schema

e29b571

add to schema config

7d085dc

clean up comments

e6559d4

sungchun12 commented Nov 18, 2022

View reviewed changes

backwards compatible nodeconfig

ca89141

sungchun12 commented Nov 18, 2022

View reviewed changes

remove comments

2c4a4cf

sungchun12 commented Nov 18, 2022

View reviewed changes

MichelleArk mentioned this pull request Jan 26, 2023

[CT-1924] Support 'contract' config outside model yaml file #6756

Closed

jtcohen6 added 2 commits January 30, 2023 14:44

Refactor functional tests

e0bcb25

Fixup formatting

dcf7062

dave-connors-3 reviewed Jan 30, 2023

View reviewed changes

Dave feedback

903a2cb

dave-connors-3 reviewed Jan 30, 2023

View reviewed changes

jtcohen6 and others added 8 commits January 30, 2023 18:15

another one - dave

c40ee92

the hits keep coming

111683a

adjust whitespace

f8b16bc

Merge branch 'main' of https://github.com/dbt-labs/dbt into dbt-const…

97f0c6b

…raints

Light touchup

2a33baf

Merge remote-tracking branch 'origin/main' into dbt-constraints

6806a7c

Add more flexibility for spark

1256e7b

Nearly there for spark

0304dbf

dbeatty10 changed the title ~~dbt Constraints~~ dbt Constraints / model contracts Feb 14, 2023

This was referenced Feb 14, 2023

[CT-1915] [Epic] Multi-project collaboration - Milestone 1 #6747

Closed

[CT-2121] Make model contracts agnostic to ordering #6975

Closed

jtcohen6 reviewed Feb 14, 2023

View reviewed changes

jtcohen6 added 2 commits February 15, 2023 00:10

Merge main

b5b1699

Try regenerating docs

d338faa

MichelleArk approved these changes Feb 15, 2023

View reviewed changes

jtcohen6 approved these changes Feb 15, 2023

View reviewed changes

jtcohen6 merged commit c647706 into main Feb 15, 2023

jtcohen6 deleted the dbt-constraints branch February 15, 2023 15:50

This was referenced Feb 27, 2023

[CT-2196] Make constraints ddl agnostic to column order #7064

Closed

[CT-2198] Unify constraints and constraints_check configs #7066

Closed

awilliamsOM1 mentioned this pull request Mar 13, 2023

Allow for Explicit Schema Definitions (DDLs) when Creating a Table / Model #2191

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dbt Constraints / model contracts #6271

dbt Constraints / model contracts #6271

sungchun12 commented Nov 16, 2022 •

edited by dbeatty10

Loading

github-actions bot commented Nov 16, 2022

github-actions bot commented Nov 16, 2022

sungchun12 commented Nov 17, 2022

sungchun12 Nov 18, 2022

sungchun12 Nov 18, 2022

jtcohen6 Nov 21, 2022

sungchun12 Nov 21, 2022 •

edited

Loading

sungchun12 Nov 18, 2022

sungchun12 Nov 18, 2022

jtcohen6 Nov 21, 2022

sungchun12 Nov 28, 2022

jtcohen6 Nov 29, 2022

sungchun12 Nov 29, 2022

sungchun12 Nov 18, 2022

sungchun12 Nov 18, 2022

sungchun12 Nov 18, 2022

jtcohen6 Nov 21, 2022

colin-rogers-dbt Jan 6, 2023

jtcohen6 commented Jan 30, 2023

dave-connors-3 Jan 30, 2023

jtcohen6 Jan 30, 2023 •

edited

Loading

dave-connors-3 Jan 30, 2023

jtcohen6 Jan 30, 2023

jtcohen6 left a comment •

edited

Loading

jtcohen6 Feb 14, 2023

jtcohen6 Feb 15, 2023

jtcohen6 Feb 14, 2023

jtcohen6 Feb 15, 2023

jtcohen6 Feb 14, 2023

sungchun12 commented Feb 15, 2023

		@@ -17,6 +17,7 @@

		from dbt.dataclass_schema import dbtClassMixin, ExtensibleDbtClassMixin

		from dbt.exceptions import CompilationException

	# Make a contract-breaking change to the model
	my_model_with_nulls = my_model_sql.replace("1 as id", "null as id")
	write_file(my_model_with_nulls, "models", "my_model.sql")

dbt Constraints / model contracts #6271

dbt Constraints / model contracts #6271

Conversation

sungchun12 commented Nov 16, 2022 • edited by dbeatty10 Loading

Description

Checklist

github-actions bot commented Nov 16, 2022

github-actions bot commented Nov 16, 2022

sungchun12 commented Nov 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sungchun12 Nov 21, 2022 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtcohen6 commented Jan 30, 2023

Choose a reason for hiding this comment

jtcohen6 Jan 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtcohen6 left a comment • edited Loading

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

sungchun12 commented Feb 15, 2023

sungchun12 commented Nov 16, 2022 •

edited by dbeatty10

Loading

sungchun12 Nov 21, 2022 •

edited

Loading

jtcohen6 Jan 30, 2023 •

edited

Loading

jtcohen6 left a comment •

edited

Loading