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

Detect breaking changes to constraints in state:modifed #7476

Merged
merged 12 commits into from
May 10, 2023

Conversation

emmyoop
Copy link
Member

@emmyoop emmyoop commented Apr 28, 2023

resolves #7065

Description

Detects breaking changes on state modified to enforced constraints. Accounts for constraints only considered enforced on table & incremental materializations.

Checklist

@cla-bot cla-bot bot added the cla:yes label Apr 28, 2023
@github-actions
Copy link
Contributor

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.

@emmyoop emmyoop changed the title Er/ct 2197 constraints modified Detect breaking changes to constraints in state:modifed May 1, 2023
@emmyoop emmyoop marked this pull request as ready for review May 4, 2023 15:11
@emmyoop emmyoop requested a review from a team as a code owner May 4, 2023 15:11
@emmyoop emmyoop requested a review from a team May 4, 2023 15:11
@emmyoop emmyoop requested review from a team as code owners May 4, 2023 15:11
@emmyoop emmyoop requested a review from VersusFacit May 4, 2023 16:04
@emmyoop emmyoop requested a review from MichelleArk May 10, 2023 15:55
@@ -608,32 +654,99 @@ def same_contract(self, old) -> bool:
contract_enforced_disabled: bool = False
columns_removed: List[str] = []
column_type_changes: List[Tuple[str, str, str]] = []
enforced_column_constraint_removed: List[List[str]] = [] # column, constraint_type
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any reason not to use a list of Tuple[str, str] here and for enforced_model_constraint_removed? Feels more consistent with the implementation so far (of columns_removed) given the nested lists are of fixed length.

Copy link
Contributor

@MichelleArk MichelleArk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple nits/suggestions, nothing blocking. Great testing! ✨

@emmyoop emmyoop merged commit 05595f5 into main May 10, 2023
@emmyoop emmyoop deleted the er/ct-2197-constraints-modified branch May 10, 2023 20:39
MichelleArk pushed a commit that referenced this pull request Jul 12, 2023
* added test that fails

* added new exception

* add partial error checking - needs more specifics

* move contract check under modelnode

* try adding only enforced constraints

* add checks for enforced constraints

* changelog

* add materialization logic

* clean up tests, tweak materializations

* PR feedback

* more PR feedback

* change to tuple
@MichelleArk MichelleArk mentioned this pull request Jul 12, 2023
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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