dbt-sqlserver icon indicating copy to clipboard operation
dbt-sqlserver copied to clipboard

[Bug] Column constraints (Primary Key, Foreign Keys, Checks, Unique) and Model constraints are not generated

Open davikor opened this issue 1 year ago • 3 comments

Currently sqlserver_adapter does not generate constraints, except for the not null constraint if it is defined as a column constraint.

The reason, as far as I understand this adapter is:

  1. sqlserver_adapter.py implements the render_model_constraintmethod but not the render_column_constraint method. When render_column_constraintis invoked the method implemented from the inherited fabric_adapterclass is invoked which in turn only supports the not null constraint, all other column constraints are ignored.
  2. dbt/include/sqlserver/macros/materializations/models//table/table.sq is missing a {{ build_model_constraints(target_relation) }} tag that is present in the fabric template and therefore the render_model_constraint method is never invoked, neither in sqlserver_adapter nor in fabric_adapter.

I did a quick test to confirm that adding these missing parts can fix this issue, however, I do not really understand the functioning and structure of the adapter/jinja templating etc. and therefore cannot provide a fix.

davikor avatar Nov 13 '24 09:11 davikor

Constraints will make the run very slow if you do it for all foreign keys for example. Use only when required.

ericmuijsvanoord avatar Nov 25 '24 09:11 ericmuijsvanoord

Constraints will make the run very slow if you do it for all foreign keys for example. Use only when required.

the point of this issue is I can add as much foreign keys as I want to my dbt project, it will never get materialized by the sql-adapter.

davikor avatar Nov 28 '24 13:11 davikor

I'm also seeing that, despite defining my constraints in YAML nothing is being generated to the model. I think short term if sqlserver is your only target the best way is to override the adapters table materialization,

{# your_dbt_project\macros\materializations\models\table\table.sql #}
{% materialization table, adapter='sqlserver' %}
  {{ log("Custom table materialization", info=True) }}

  {%- set existing_relation = load_cached_relation(this) -%}
  {%- set target_relation = this.incorporate(type='table') %}
  {%- set intermediate_relation =  make_intermediate_relation(target_relation) -%}
  -- the intermediate_relation should not already exist in the database; get_relation
  -- will return None in that case. Otherwise, we get a relation that we can drop
  -- later, before we try to use this name for the current operation
  {%- set preexisting_intermediate_relation = load_cached_relation(intermediate_relation) -%}
  /*
      See ../view/view.sql for more information about this relation.
  */
  {%- set backup_relation_type = 'table' if existing_relation is none else existing_relation.type -%}
  {%- set backup_relation = make_backup_relation(target_relation, backup_relation_type) -%}
  -- as above, the backup_relation should not already exist
  {%- set preexisting_backup_relation = load_cached_relation(backup_relation) -%}
  -- grab current tables grants config for comparision later on
  {% set grant_config = config.get('grants') %}

  -- drop the temp relations if they exist already in the database
  {{ drop_relation_if_exists(preexisting_intermediate_relation) }}
  {{ drop_relation_if_exists(preexisting_backup_relation) }}

  {{ run_hooks(pre_hooks, inside_transaction=False) }}

  -- `BEGIN` happens here:
  {{ run_hooks(pre_hooks, inside_transaction=True) }}

  -- build model
  {% call statement('main') -%}
    {{ get_create_table_as_sql(False, intermediate_relation, sql) }}
  {%- endcall %}

  -- cleanup
  {% if existing_relation is not none %}
     /* Do the equivalent of rename_if_exists. 'existing_relation' could have been dropped
        since the variable was first set. */
    {% set existing_relation = load_cached_relation(existing_relation) %}
    {% if existing_relation is not none %}
        {{ adapter.rename_relation(existing_relation, backup_relation) }}
    {% endif %}
  {% endif %}

  {{ adapter.rename_relation(intermediate_relation, target_relation) }}

  {% do create_indexes(target_relation) %}

  {{ run_hooks(post_hooks, inside_transaction=True) }}

  {% set should_revoke = should_revoke(existing_relation, full_refresh_mode=True) %}
  {% do apply_grants(target_relation, grant_config, should_revoke=should_revoke) %}

  {% do persist_docs(target_relation, model) %}

  -- `COMMIT` happens here
  {{ adapter.commit() }}

  -- finally, drop the existing/backup relation after the commit
  {{ drop_relation_if_exists(backup_relation) }}

  {# New! Added build_model_constraints #}
  {{ build_model_constraints(target_relation) }}

  {{ run_hooks(post_hooks, inside_transaction=False) }}

  {{ return({'relations': [target_relation]}) }}
{% endmaterialization %}

I think you also need a helper:

{# dbt-your_dbt_project\macros\materializations\models\table\columns_spec_ddl.sql #}
{% macro build_model_constraints(relation) %}
    {{ log("Custom Constraints Macro: build_model_constraints", info=True) }}
    {{ return(adapter.dispatch('build_model_constraints')(relation)) }}
{% endmacro %}

{% macro sqlserver__build_model_constraints(relation) %}
  {{ log("Custom Constraints Macro: sqlserver__build_model_constraints", info=True) }}
  {# loop through user_provided_columns to create DDL with data types and constraints #}
    {%- set raw_model_constraints = adapter.render_raw_model_constraints(raw_constraints=model['constraints']) -%}
    {% for c in raw_model_constraints -%}
      {% set alter_table_script %}
        alter table {{ relation.include(database=False) }} {{c}};
      {%endset%}
      {% call statement('alter_table_add_constraint') -%}
        {{alter_table_script}}
      {%- endcall %}
    {% endfor -%}
{% endmacro %}

In my testing (dbt version 1.8.9) defining constraints at the model level seems to be working now. At the column level -not_null is the only one being applied.

TWDickson avatar May 01 '25 16:05 TWDickson