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-2402] [Feature] Improve warnings/errors for constraints <> materialization types #7335

Closed
Tracked by #7372
jtcohen6 opened this issue Apr 12, 2023 · 2 comments · Fixed by #7696
Closed
Tracked by #7372
Assignees
Labels
enhancement New feature or request model_contracts multi_project paper_cut A small change that impacts lots of users in their day-to-day
Milestone

Comments

@jtcohen6
Copy link
Contributor

jtcohen6 commented Apr 12, 2023

Views

If your model is materialized as a view and has constraints defined, those constraints are not supported & will be ignored, on ALL data platforms. We should raise a warning saying this, similar to the warn_unsupported functionality added in #7250.

Update: This might require calling a different macro within create_view_as, since all constraint parsing/rendering is currently happening within create_table_asget_table_columns_and_constraints.


Ephemeral models

Parsing Error
  Constraint validation failed for: (models/sometable.sql)
  Only table, view, and incremental materializations are supported for constraints, but found 'ephemeral'

Let's just update the language here: "constraint validation" → "contract enforcement"

def validate_constraint_prerequisites(self, model_node: ModelNode):
errors = []
if not model_node.columns:
errors.append(
"Constraints must be defined in a `yml` schema configuration file like `schema.yml`."
)
if model_node.config.materialized not in ["table", "view", "incremental"]:
errors.append(
f"Only table, view, and incremental materializations are supported for constraints, but found '{model_node.config.materialized}'"
)
if str(model_node.language) != "sql":
errors.append(f"Language Error: Expected 'sql' but found '{model_node.language}'")
if errors:
raise ParsingError(
f"Constraint validation failed for: ({model_node.original_file_path})\n"
+ "\n".join(errors)
)

@jtcohen6 jtcohen6 added enhancement New feature or request paper_cut A small change that impacts lots of users in their day-to-day multi_project model_contracts labels Apr 12, 2023
@github-actions github-actions bot changed the title Improve warnings/errors for constraints <> materialization types [CT-2402] Improve warnings/errors for constraints <> materialization types Apr 12, 2023
@jtcohen6 jtcohen6 added this to the v1.5.x milestone Apr 18, 2023
@jtcohen6 jtcohen6 changed the title [CT-2402] Improve warnings/errors for constraints <> materialization types [CT-2402] [Feature] Improve warnings/errors for constraints <> materialization types Apr 30, 2023
@emmyoop
Copy link
Member

emmyoop commented May 3, 2023

Doing this at parse time where we do model config/property validation makes more sense than creating a new macro that has to be implemented on all adapters.

@jtcohen6
Copy link
Contributor Author

jtcohen6 commented May 3, 2023

Let's review the validate_constraint_prerequisites method as well, see which validation we're actually still using/needing, and either remove or update the warnings accordingly

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 paper_cut A small change that impacts lots of users in their day-to-day
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants