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

Improve Deduplicate Macro to use QUALIFY #543

Closed
codigo-ergo-sum opened this issue Apr 8, 2022 · 5 comments · Fixed by #548
Closed

Improve Deduplicate Macro to use QUALIFY #543

codigo-ergo-sum opened this issue Apr 8, 2022 · 5 comments · Fixed by #548
Labels
enhancement New feature or request

Comments

@codigo-ergo-sum
Copy link
Contributor

Describe the feature

The deduplicate macro currently uses a combination of dbt_utils.star and a subquery to work around needing to filter based on the result of a window function but not wanting to return the filtering column used. The QUALIFY keyword, recently introduced in Snowflake and BigQuery, allows for filtering the result of a query directly on a window function in a cleaner way.

This current code:

    select
        {{ dbt_utils.star(relation, relation_alias='deduped') | indent }}
    from (
        select
            _inner.*,
            row_number() over (
                partition by {{ group_by }}
                {% if order_by is not none -%}
                order by {{ order_by }}
                {%- endif %}
            ) as rn
        from {{ relation if relation_alias is none else relation_alias }} as _inner
    ) as deduped
    where deduped.rn = 1

I think could look like this:

       select
            *
        from {{ relation if relation_alias is none else relation_alias }} deduped
        qualify
            row_number() over (
                partition by {{ group_by }}
                {% if order_by is not none -%}
                order by {{ order_by }}
                {%- endif %}
            ) = 1

Additional context

Although BQ does support qualify, it also has issues with window functions with too much data choking on single nodes, hence why the BQ override for the macro uses array_agg instead. And Redshift and potentially other databases don't support QUALIFY. But this could at least be overridden more cleanly and probably more performantly for Snowflake.

Who will this benefit?

What kind of use case will this feature be useful for? Please be specific and provide examples, this will help us prioritize properly.

Are you interested in contributing this feature?

Possibly, depends on time and what the dbt_utils build process looks like these days? Last time I submitted a fix a few months ago it definitely required some help from the team in troubleshooting the build process.

@codigo-ergo-sum codigo-ergo-sum added enhancement New feature or request triage labels Apr 8, 2022
@judahrand
Copy link
Contributor

I suspect that this will still mean that a Relation has to be passed though, right? In order to be compatible with Redshift + Postgres.

@judahrand
Copy link
Contributor

I've implemented this (along with some other non-breaking improvements) in #549.

@judahrand
Copy link
Contributor

judahrand commented Apr 14, 2022

@codigo-ergo-sum This isn't entirely relevant to this issue but having taken a look at this for other DBs as well I've come to the conclusion that SQL dialects are even more annoying and limiting than I'd thought. The actual ANSI way of doing this deduplication is so rarely supported that I didn't even know it existed! As far as I can tell only Trino and PG13+ actually support it (maybe others). The ANSI way of doing this dedupe is:

select *
from {{ relation if relation_alias is none else relation_alias }}
order by row_number() over (
    partition by {{ group_by }}
    order by {{ order_by }}
) fetch first row with ties

And even though Snowflake explicitly claims to support the ANSI FETCH syntax here... They don't support WITH TIES!!!!!

@judahrand
Copy link
Contributor

This can be closed now that #548 has been merged, I believe.

@dbeatty10
Copy link
Contributor

This can be closed now that #548 has been merged, I believe.

Thank you for calling this out @judahrand ! Added "Resolves #543" as a comment into #548 for traceability and manually closing this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants