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-2199] Generalize constraint compatibility warnings #7067

Closed
Tracked by #6747
MichelleArk opened this issue Feb 27, 2023 · 4 comments · Fixed by #7250
Closed
Tracked by #6747

[CT-2199] Generalize constraint compatibility warnings #7067

MichelleArk opened this issue Feb 27, 2023 · 4 comments · Fixed by #7250
Assignees
Labels
model_contracts multi_project user docs [docs.getdbt.com] Needs better documentation

Comments

@MichelleArk
Copy link
Contributor

MichelleArk commented Feb 27, 2023

Refinement discussion: #6750

Currently, constraints that are set but unenforceable or unsupported raise warnings in adapter-specific jinja macros. For example: snowflake__get_columns_spec_ddl in dbt-snowflake.

To generalize this warning mechanism, we'll need a new adapter method that can tell dbt whether a constraint is:

  1. supported & enforced
  2. supported but not enforced
  3. not supported

Here's what that might look like for dbt-snowflake, in pseudo code:

@staticmethod
def constraint_support:
    return {
        "not_null": ConstraintSupport.ENFORCED,
        "primary_key": ConstraintSupport.NOT_ENFORCED,
        "foreign_key": ConstraintSupport.NOT_ENFORCED,
        "unique": ConstraintSupport.NOT_ENFORCED,
        "check": ConstraintSupport.NOT_SUPPORTED,
    }

This mapping could be a static method, or a property, or maybe just a constant class attribute. As long as subclasses of BaseAdapters are able to overwrite it. This mapping should also not be available in the jinja context as it's intended for use within the python adapter implementation and not by end users in user space (jinja).

This method would be called in order to determine whether to raise warnings for unenforced/unsupported constraints (unless the user has disabled via warn_unsupported or warn_unenforced (#7066)). Ideally, warnings would be raised at runtime - during materialization, for legible user-facing logs. The actual method that raises warnings could also be implemented in python, on the adapter, calling warn_or_error and firing an event under the hood. It should be used to replace the existing adapter-specific warning mechanisms.

The constraint_support method should be implemented for each adapter, but the warning implementation should be generalized and only implemented in dbt-core.


Is blocked by:

Blocks:

@github-actions github-actions bot changed the title Introduce constraint_support method on base adapter [CT-2199] Introduce constraint_support method on base adapter Feb 27, 2023
@MichelleArk MichelleArk changed the title [CT-2199] Introduce constraint_support method on base adapter [CT-2199] Generalize constraint compatability warnings Feb 27, 2023
@jtcohen6
Copy link
Contributor

jtcohen6 commented Feb 27, 2023

Adapter-specific support

The default implementation of this method should be the one shown in the description above. This should hold true for dbt-redshift and dbt-snowflake, and seems to be the standard pattern for most analytical data platforms.

constraint type support
not_null enforced
primary_key not enforced
foreign_key not enforced
unique not enforced
check not supported

For dbt-bigquery:
Updated given recent BQ support for primary_key + foreign_key

constraint type support
not_null enforced
primary_key not supported supported
foreign_key not supported supported
unique not supported
check not supported

For dbt-postgres:

constraint type support
not_null enforced
primary_key enforced
foreign_key enforced
unique enforced
check enforced

For dbt-spark/dbt-databricks: not_null and check constraints are supported and enforced. In the current implementation, however, none of these constraints is enforced before a model builds. I see this more as a platform limitation that we should document, and work with the Databricks team to improve. Still, we shouldn't include these as part of the "model contract" while they aren't things we can actually enforce at build time. So, for now, we should consider them "supported, not enforced." In a future version where we can supply column specs within atomic create table as statements, we can flip them on as enforced.

constraint type support
not_null not enforced
primary_key not enforced
foreign_key not enforced
unique not enforced
check not enforced

This method doesn't include anything for the type: custom of ConstraintOption, which we're including in the spec (#7066). Whatever that might be, it's bound to be specific to someone's very special data platform, and the adapter for that data platform should include it in this method.

@peterallenwebb
Copy link
Contributor

peterallenwebb commented Mar 15, 2023

Is there any need to distinguish "supported/enforced on a column" from "supported/enforced on a model" for the purposes of this case, or is that reliably a none-or-both proposition across databases?

@jtcohen6
Copy link
Contributor

Is there any need to distinguish "supported on a column" from "supported on a model" for the purposes of this case, or is that reliably a none-or-both proposition across databases?

Good thought! It's reliably a none-or-both proposition.

(When I first read your question, I thought it was asking something slightly different: Are there any constraints that cannot be defined at both column & model level? The answer there is "yes," not_null constraints can only be defined at the column level.)

@jtcohen6
Copy link
Contributor

If specified, a custom-type constraint should always be presumed to be supported and not enforced

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

4 participants