duckdb / dbt-duckdb

dbt (http://getdbt.com) adapter for DuckDB (http://duckdb.org)
Apache License 2.0
816 stars 69 forks source link

Check constraint does not include parentheses #156

Closed Bilbottom closed 1 year ago

Bilbottom commented 1 year ago

This may be by design (or a dbt-core issue) -- in which case, please close this 😝

Summary

The new constraints option in dbt 1.5 adds extra options to the DDL

The dbt-core docs imply that the expression for the CHECK constraint does not need to include parentheses:

image

When using the CHECK constraint in dbt-duckdb (v1.5.0), the parentheses are not added which causes a syntax error:

image

Under the Hood

For the CHECK constraint, dbt-core prefixes the condition with the check keyword:

@classmethod
def render_column_constraint(cls, constraint: ColumnLevelConstraint) -> Optional[str]:
    """Render the given constraint as DDL text. Should be overriden by adapters which need custom constraint
    rendering."""
    if constraint.type == ConstraintType.check and constraint.expression:
        return f"check {constraint.expression}"
    elif constraint.type == ConstraintType.not_null:
        ...

Source: https://github.com/dbt-labs/dbt-core/blob/f1dddaa6e96b743498440925a55e66a7d5cdacfa/core/dbt/adapters/base/impl.py#L1306-L1312

In this duckdb adaptor, the CHECK constraint just calls the dbt-core method:

https://github.com/jwills/dbt-duckdb/blob/dfb89f89050cec35562487c5c27d68af1759f609/dbt/adapters/duckdb/impl.py#L205-L212

However, duckdb requires parentheses to be included around the check expression (see the CREATE TABLE syntax graph):

image

Of course, having the user add the parentheses into the expression would solve this -- but adding this might not be obvious to many people, especially given the usage implied by the dbt-core docs

A solution for this is to add the parentheses in this adapter's render_column_constraint method:

def render_column_constraint(cls, constraint: ColumnLevelConstraint) -> Optional[str]: 
     """Render the given constraint as DDL text. Should be overriden by adapters which need custom constraint 
     rendering.""" 
     if constraint.type == ConstraintType.foreign_key: 
         # DuckDB doesn't support 'foreign key' as an alias 
         return f"references {constraint.expression}"
    elif constraint.type == ConstraintType.check:
        # DuckDB requires parentheses around the check constraint's expression
        return f"check ({constraint.expression})"
    else: 
        return super().render_column_constraint(constraint) 
jwills commented 1 year ago

@Bilbottom thank you for the phenomenally good issue! My read here is that the CHECK behavior here with the parentheses around the expression is required in Postgres (so it's not a DuckDB-specific thing, just something DuckDB inherits b/c it uses the same parser as Postgres: https://www.postgresql.org/docs/current/ddl-constraints.html )

I'm gonna cc @jtcohen6 and @dbeatty10 on this b/c I'm reasonably sure that we should open this issue on dbt-core and fix it upstream there.

Is this causing a problem for you in your day job right now, or were you just poking around at the new functionality? I fully expect that I will cut a 1.5.1 of dbt-duckdb on Monday or Tuesday of this week, and I would be happy to include a fix for this then, though as you said we do have the workaround for the check issue right now.

Bilbottom commented 1 year ago

@jwills Haha thank you -- this is just me poking at new functionality, not a work requirement, so no rush for this to be resolved 🙂

jtcohen6 commented 1 year ago

Super thorough!! Glad to have you poking around @Bilbottom :)

Agree with fixing this in dbt-core, by adding some parens right here

Bilbottom commented 1 year ago

Closing this since this will be resolved in dbt-core instead: