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

Upgrade dbt-utils #7

Merged
merged 7 commits into from
Oct 12, 2022
Merged

Upgrade dbt-utils #7

merged 7 commits into from
Oct 12, 2022

Conversation

mdesmet
Copy link
Contributor

@mdesmet mdesmet commented Sep 28, 2022

No description provided.

@hovaesco
Copy link

hovaesco commented Oct 3, 2022

How about #4? Should it be closed?

@mdesmet mdesmet force-pushed the bug/fix-adapter-macros branch 2 times, most recently from 5648dc3 to d8d6f43 Compare October 5, 2022 07:02
@mdesmet mdesmet force-pushed the bug/fix-adapter-macros branch from d8d6f43 to 4f513f6 Compare October 5, 2022 07:05
@mdesmet mdesmet changed the title Apply adapter macro fixes on dbt-utils Upgrade dbt-utils Oct 5, 2022
# TODO: change with release version
RUN pip install git+https://github.com/starburstdata/dbt-trino.git
# TODO: change with release version once PR is merged
RUN pip3 install git+https://github.com/mdesmet/dbt-trino.git@feature/get_colums_in_relation
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be changed after we merge that branch on dbt-trino

@mdesmet mdesmet requested a review from hovaesco October 5, 2022 08:11

select * from filtered

{% endmacro %}
Copy link

Choose a reason for hiding this comment

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

nit: new line here and in other places

{% macro trino__date_spine(datepart, start_date, end_date) %}


{# call as follows:
Copy link

Choose a reason for hiding this comment

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

Is the comment needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it comes from the original dbt implementation, I would say it's ok to keep it.

{%- macro trino__deduplicate(relation, partition_by, order_by) -%}
{#
-- This is a temporary solution until https://github.com/trinodb/trino/issues/14455 is resolved
-- It leaks the rn column compared to the NATURAL JOIN solution from dbt
Copy link

Choose a reason for hiding this comment

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

What it means rn?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the NATURAL JOIN implementation (which is not supported) would not leak the row number column as output of the deduplicate query.

@mdesmet mdesmet force-pushed the bug/fix-adapter-macros branch 2 times, most recently from ce00d7f to 24836a5 Compare October 12, 2022 06:44
@mdesmet mdesmet merged commit 2fc3d70 into main Oct 12, 2022
@mdesmet mdesmet deleted the bug/fix-adapter-macros branch October 12, 2022 06:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants