Skip to content
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

[CT-2197] [Feature] Detect breaking changes to constraints in state:modified check #7065

Closed
Tracked by #7372 ...
MichelleArk opened this issue Feb 27, 2023 · 8 comments · Fixed by #7476
Closed
Tracked by #7372 ...
Assignees
Labels
enhancement New feature or request model_contracts multi_project user docs [docs.getdbt.com] Needs better documentation

Comments

@MichelleArk
Copy link
Contributor

MichelleArk commented Feb 27, 2023

If a model with contract: True has its constraints modified in a way that represents a breaking change, dbt should raise an error as part of the state:modified check.

What constitutes a breaking change?

  • Removing a (column or model-level) constraint that is enforced by the data warehouse
  • Changing the materialization from table to view or ephemeral on a model that has at least one enforced constraint.

Blocked by:

@github-actions github-actions bot changed the title Detect breaking changes to constraints in state:modified check [CT-2197] Detect breaking changes to constraints in state:modified check Feb 27, 2023
@jtcohen6
Copy link
Contributor

Also a breaking change: You have a model with at least one constraint defined that the data platform can enforce. You switch the materialization of that model from table or incremental, to view — because data platforms do not support constraints on views.

@MichelleArk
Copy link
Contributor Author

MichelleArk commented Mar 3, 2023

You switch the materialization of that model from table or incremental, to view

or to ephemeral!

@jtcohen6
Copy link
Contributor

jtcohen6 commented Mar 3, 2023

Good point! We really shouldn't allow ephemeral models to be contract: true or access: public in the first place

@emmyoop
Copy link
Member

emmyoop commented Mar 6, 2023

We are unclear on exactly what we would be checking has changed and how often. There's concern this would cause a slowdown in detecting changes if we have to check every single column for every model with a contract.

@sadahry
Copy link

sadahry commented Apr 12, 2023

We think this feature will be useful for us,
so I'd like to ask you about this spec.

We've tried using dbt-core==1.5.0-b5 ,
and found disallowing adding new columns to yml in this version.

e.g.,

before

version: 2

models:
  - name: sometable
    config:
      contract:
        enforced: true
    columns:
      - name: order_id
        data_type: number

      - name: order_name
        data_type: varchar

after -> then failed with dbt.exceptions.ModelContractError

version: 2

models:
  - name: sometable
    config:
      contract:
        enforced: true
    columns:
      - name: order_id
        data_type: number

      - name: order_name
        data_type: varchar

      - name: categories
        data_type: varchar

This is what you excepted?

I read it and found this description.

Additive changes are not considered breaking:

My understanding is that it means allow adding columns in yaml, but current behavior does not.
Allowing adding columns is useful for us because our model's are updated in a fast cycle.

I'd like to know if this specification is as expected
or if there is a mistake in me or dbt-core.

@jtcohen6
Copy link
Contributor

jtcohen6 commented Apr 12, 2023

@sadahry Thanks for trying out the beta, and for the clear reproduction case! I agree that's not the intended behavior. It should be possible to add a new column without raising a ModelContractError (breaking change). I'll open a bug report now.

@jtcohen6
Copy link
Contributor

jtcohen6 commented Apr 12, 2023

@sadahry This will be fixed in #7333. Thank you so much for catching this! I'm very glad we were able to get the behavior working as intended before cutting the v1.5 release candidate.

We'll be swinging back to this issue for v1.6, to add in the scope of raising an exception on breaking changes to constraints. That is, any removals or modifications — but additions are totally okay!

@sadahry
Copy link

sadahry commented Apr 13, 2023

Thank you for your quick fix! And looking forward to the enhancement for v1.6 👍

@jtcohen6 jtcohen6 added the enhancement New feature or request label Apr 30, 2023
@jtcohen6 jtcohen6 changed the title [CT-2197] Detect breaking changes to constraints in state:modified check [CT-2197] [Feature] Detect breaking changes to constraints in state:modified check Apr 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request model_contracts multi_project user docs [docs.getdbt.com] Needs better documentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants