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

Incremental does not work with unique_key that is array #241

Closed
johnnytang24 opened this issue Jun 3, 2022 · 6 comments · Fixed by #251
Closed

Incremental does not work with unique_key that is array #241

johnnytang24 opened this issue Jun 3, 2022 · 6 comments · Fixed by #251
Assignees
Labels
bug Something isn't working bugfix good first issue Good for newcomers
Milestone

Comments

@johnnytang24
Copy link

johnnytang24 commented Jun 3, 2022

Example model:

{{ 
  config
  (
    materialized='incremental'
    , unique_key=['A', 'B']
  ) 
}}
WITH CTE AS
(
  SELECT 1 AS A, 2 AS B
  UNION
  SELECT 3 AS A, 4 AS B
)
SELECT
  A
  , B
FROM
  CTE

Error: ('42000', "[42000] [Microsoft][ODBC Driver 17 for SQL Server][SQL Server]Incorrect syntax near 'using'. (102) (SQLExecDirectW);

Code generated in run/project/model:

delete from "Test"."dbt"."t"
        using "Test"."dbt"."#t__dbt_tmp"
        where (
                "Test"."dbt"."#t__dbt_tmp".A = "Test"."dbt"."t".A
                and
                "Test"."dbt"."#t__dbt_tmp".B = "Test"."dbt"."t".B
        );

DELETE FROM ... USING is not supported by SQL Server

@johnnytang24
Copy link
Author

This fixes it:

File: dbt/include/sqlserver/macros/materializations/models/incremental/merge.sql

{% macro sqlserver__get_delete_insert_merge_sql(target, source, unique_key, dest_columns) %}

    {%- set dest_cols_csv = get_quoted_csv(dest_columns | map(attribute="name")) -%}

    {% if unique_key %}
        {% if unique_key is sequence and unique_key is not string %}
            delete from {{ target }}
            where exists (
                SELECT NULL
                FROM
                  {{ source }}
                WHERE
                {% for key in unique_key %}
                    {{ source }}.{{ key }} = {{ target }}.{{ key }}
                    {{ "and " if not loop.last }}
                {% endfor %}
            );
        {% else %}
            delete from {{ target }}
            where (
                {{ unique_key }}) in (
                select ({{ unique_key }})
                from {{ source }}
            );

        {% endif %}
        {% endif %}

    insert into {{ target }} ({{ dest_cols_csv }})
    (
        select {{ dest_cols_csv }}
        from {{ source }}
    )

{% endmacro %}

@sdebruyn
Copy link
Member

sdebruyn commented Jun 3, 2022

@johnnytang24 Thanks for immediately providing us with the fix! Could you quickly also give me the output of dbt --version? Thanks!

@sdebruyn sdebruyn self-assigned this Jun 3, 2022
@sdebruyn sdebruyn added bug Something isn't working good first issue Good for newcomers bugfix labels Jun 3, 2022
@johnnytang24
Copy link
Author

Core:
  - installed: 1.1.0

@sdebruyn sdebruyn added this to the v1.1.0 milestone Jun 4, 2022
@sdebruyn sdebruyn removed their assignment Jun 4, 2022
@sdebruyn
Copy link
Member

sdebruyn commented Jun 6, 2022

While going through the dbt docs, I could not find any documented behaviour where you can use an array as the unique key. All examples point to SQL columns or expressions. The unique_key for incremental models is not properly documented, but you can find the same property for snapshot configurations: https://docs.getdbt.com/reference/resource-configs/unique_key

There it says you can use an expression to combine multiple columns.

However... when you take a look at the core used in dbt-core it does seem to support sequences so I'd say we have to do the same here.

I'll do the fix in this project and create an issue for the dbt documentation.

@sdebruyn
Copy link
Member

sdebruyn commented Jun 6, 2022

@johnnytang24 Fix is ready in pr #251, care to review?

@johnnytang24
Copy link
Author

Looks good!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working bugfix good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants