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.

Sign up for GitHub

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

Add Snowflake-specific implementation of min_or_max #32

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
+19 −0
Open

Add Snowflake-specific implementation of min_or_max #32

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ed4483c
tmp snowflake min or max
Apr 5, 2023
f4f6c10
add snowflake min or max
Apr 7, 2023
982270e
update if then to dispatch
Apr 21, 2023
fa808f2
formatting
Apr 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions macros/utils/aggregations/_min_or_max.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
{% macro _min_or_max(min_or_max, qualified_col) %}
{{ return(adapter.dispatch('_min_or_max','dbt_activity_schema')(min_or_max, qualified_col)) }}
{%- endmacro -%}

{% macro default___min_or_max(min_or_max, qualified_col) -%}

{% set aggregation = "min" if min_or_max == "min" else "max" %}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this assignment should be via a dispatched macro. And then we can call that aggregation with {{ }} in the relevant place.

Copy link
Author

@wylbee wylbee Apr 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tnightengale Apologies for the back and forth. I think I'm missing something here that would help me address this feedback. Three questions for you:

  1. I follow how the default _min_or_max macro works- per the discussion in TRY_CAST SQL compilation error #26, this code uses built-in Snowflake functionality to achieve the same end rather than replicate the prepend approach. In my dbt project and the Snowflake version of the integration tests, I see that the min_by/max_by version produces the expected results. Could you explain further what you mean by I don't think this will work as is??
  2. For So I think that logic needs to be replicated for Snowflake as well, as far as I understand., are you saying that you would prefer a Snowflake version of the prepend logic rather than the native Snowflake approach used here or something else?
  3. I'm not following the comment on the assignment via dispatch macro. The version here in this PR is essentially a carbon copy of the example provided in the dbt docs for this functionality. Could you explain more about what you are looking for or point me to an example on which I can base the refactoring?

Appreciate the feedback to give me what I need to get this all squared away!

{% set column_name = qualified_col.split(".")[-1].strip() %}
Expand Down Expand Up @@ -54,3 +58,18 @@
{% do return(output) %}

{% endmacro %}

{% macro snowflake___min_or_max(min_or_max, qualified_col) -%}
{% set aggregation = "min_by" if min_or_max == "min" else "max_by" %}
{% set qualified_ts_col = "{}.{}".format(
dbt_activity_schema.appended(), dbt_activity_schema.columns().ts
) %}

{# Apply min or max by to the selected column. #}
{% set aggregated_col %}
{{ aggregation }}({{ qualified_col }}, {{ qualified_ts_col }})
{% endset %}

{# Return output. #}
{% do return(aggregated_col) %}
{%- endmacro %}