-
Notifications
You must be signed in to change notification settings - Fork 960
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
Adapter constraint support tables #3944
Conversation
✅ Deploy Preview for docs-getdbt-com ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey @matthewshaver , looks awesome and i left some suggestions! do you think you should link to the #platform-constraint-support in the 'what are contracts supported' section? (also 'what are contracts supported' sounds a bit weird)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@matthewshaver I double-checked the constraints support within this PR against all of these:
- ✅ dbt-redshift
- ✅ dbt-snowflake
- ✅ dbt-bigquery
- ✅ dbt-postgres
- ✅ dbt-spark
- ✅ dbt-databricks doesn't override
CONSTRAINT_SUPPORT
, so it inherits the settings from dbt-spark
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@matthewshaver and @mirnawong1
In order to figure out the best language to use in the tables, see below for some explanations. "supported; not enforced" seems to be the most tricky one to explain and understand.
If we change the verbiage, it seems that "supported but not enforced" would be better than "supported and not enforced".
🌕 supported & enforced
The model won't even build if it violates this constraint.
🌗 supported; not enforced
The platform supports specifying this type of constraint, but a model can still build even if it violates it. It's basically a "fake" constraint only for metadata purposes. This is common for modern cloud data warehouses and less common for legacy databases.
🌑 not supported
You can't specify this type of constraint for this platform.
that's awesome,i think it would be really helpful to add those definitions so users know what they mean, just to be extra clear. i had to scroll back and forth to try to get my bearings so maybe having it clear before the table is helpful for users. i like support but not enforced! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@matthewshaver and @mirnawong1
Would an alternative like this be more clear?
constraint type | platform support | platform enforcement |
---|---|---|
not_null | ✅ supported | ✅ enforced |
primary_key | ✅ supported | ❌ not enforced |
foreign_key | ✅ supported | ❌ not enforced |
unique | ✅ supported | ❌ not enforced |
check | ❌ not supported | ❌ not enforced |
Rather than this?
constraint type | support |
---|---|
not_null | 🌕 supported & enforced |
primary_key | 🌗 supported; not enforced |
foreign_key | 🌗 supported; not enforced |
unique | 🌗 supported; not enforced |
check | 🌑 not supported |
yep i prefer that @dbeatty10 ! i was going to suggest changing the table but i didn't want to mess anything going on between yall. separating them out is clearer! i still think having a paragraph explaining what supported and enforced or not enforced means in this context is helpful -- just to be super clear with users. |
Co-authored-by: mirnawong1 <[email protected]>
@dbeatty10 Thank you for that context - much clearer and that makes the tables easier to follow. Will implement. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@matthewshaver Just some wording suggestions to simplify the sentences a little
Co-authored-by: Leona B. Campbell <[email protected]>
Co-authored-by: Leona B. Campbell <[email protected]>
</TabItem> | ||
<TabItem value="Spark" label="Spark"> | ||
|
||
Currently, `not_null` and `check` constraints are supported and enforced only after a model builds. Because of this platform limitation, dbt considers these constraints `supported` but `not enforced`, which means they're not part of the "model contract" since these constraints can't be enforced at build time. This table will change as the features evolve. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like the identical Databricks copy below, this paragraph is confusing.
I'd suggest rewriting for more clarity, potentially like the following:
Currently,
not_null
andcheck
constraints are applied only after a model builds. Because of this platform limitation, dbt considers these constraintssupported
butnot enforced
, which means they're not part of the "model contract" since these constraints can't be enforced at build time. This table will change as the features evolve.
</TabItem> | ||
<TabItem value="Databricks" label="Databricks"> | ||
|
||
Currently, `not_null` and `check` constraints are supported and enforced only after a model builds. Because of this platform limitation, dbt considers these constraints `supported` but `not enforced`, which means they're not part of the "model contract" since these constraints can't be enforced at build time. This table will change as the features evolve. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like the identical Spark copy above, this paragraph is confusing.
I'd suggest rewriting for more clarity, potentially like the following:
Currently,
not_null
andcheck
constraints are applied only after a model builds. Because of this platform limitation, dbt considers these constraintssupported
butnot enforced
, which means they're not part of the "model contract" since these constraints can't be enforced at build time. This table will change as the features evolve.
- **Supported and enforced** — The model won't build if it violates the constraint. | ||
- **Supported and not enforced** — The platform supports specifying the type of constraint, but a model can still build even if building the model violates the constraint. This constraint exists for metadata purposes only. This is common for modern cloud data warehouses and less common for legacy databases. | ||
- **Not supported and not enforced** — You can't specify the type of constraint for the platform. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice if the bolded descriptions were more readable. Right now, the last one feels too verbose and the middle one could be more precise.
Would the following in conjunction with the split columns be clear enough?
- Enforced or Supported and enforced or Supported & enforced
- Supported (but not enforced) or Supported but not enforced or Supported; not enforced
- Not supported or Not supported (and not enforced)
#3944 (review) begins the original discussion about this.
Preview
What are you changing in this pull request and why?
Adding tables outlining adapter-specific constraint support to the model contracts page
Checklist