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

added macro for wh selection #503

Merged
merged 10 commits into from
Mar 8, 2023
Merged

added macro for wh selection #503

merged 10 commits into from
Mar 8, 2023

Conversation

McKnight-42
Copy link
Contributor

@McKnight-42 McKnight-42 commented Mar 7, 2023

resolves #438

Description

take over pr: #115 to get pre-commit issues taken care of and ready for merge

Checklist

@McKnight-42 McKnight-42 requested a review from a team as a code owner March 7, 2023 20:35
@McKnight-42 McKnight-42 self-assigned this Mar 7, 2023
@cla-bot cla-bot bot added the cla:yes label Mar 7, 2023
@McKnight-42 McKnight-42 mentioned this pull request Mar 7, 2023
3 tasks
@@ -73,6 +76,8 @@ def _get_warehouse(self) -> str:

def _use_warehouse(self, warehouse: str):
"""Use the given warehouse. Quotes are never applied."""
kwargs = {"warehouse": warehouse}
warehouse = self.execute_macro(SNOWFLAKE_WAREHOUSE_MACRO_NAME, kwargs=kwargs) # type: ignore
Copy link
Contributor Author

Choose a reason for hiding this comment

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

curious if anyone has an idea as to why this causes a
dbt/adapters/snowflake/impl.py:80: error: Incompatible types in assignment (expression has type "AttrDict", variable has type "str") [assignment] error? seems same as all implementations in dbt-core I could find. just curious if there is better path than the ignore

Copy link
Contributor

Choose a reason for hiding this comment

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

It's silly, but you can remove the type annotation from warehouse and mypy will be happy; you can also update it to Any. The issue is that self.execute_macro() lists the type for kwargs as Dict[str, Any]. mypy doesn't like it when you make the type more strict (str is a proper subset of Any).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh that's interesting iI didn't think the type value for the "warehouse" input would bother the previously defined one for execute_macro thank you for clearing that up @mikealfare

@@ -0,0 +1,3 @@
kind: Added
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a supported format? I feel like our process to roll up change logs might not like this. Based on the timestamp, it might be an older format.

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 part might be from before some more recent changes to our changie logic so yeah can remove

@McKnight-42 McKnight-42 merged commit 2e061fc into main Mar 8, 2023
@McKnight-42 McKnight-42 deleted the mcknight/take-over-103 branch March 8, 2023 18:55
@manugarri
Copy link

manugarri commented Mar 17, 2023

@McKnight-42 nice! after some trial and error I managed to get it working on prerelease v1.5.0b3 , is there any timeline to when is gonna be released on pypi?

@McKnight-42
Copy link
Contributor Author

@manugarri we did some releases yesterday actually https://pypi.org/project/dbt-snowflake/1.5.0b3/ I believe this should be apart of that release

@manugarri
Copy link

manugarri commented Mar 17, 2023

Awesome, great! One question I have, is it not possible to access environment variables inside this macro?

im trying to return different warehouses depending on the snowflake user running dbt, this is what i have so far

    {% set warehouses_specs = {
            "user1": {
                "SMALL": "wh1_small",
                "LARGE": "wh1_large"
            },
            "user2": {
                "SMALL": "wh2_small",
                "LARGE": "wh2_large"
            }
        }
    %}

    {{ return(warehouses_specs[env_var('SNOWFLAKE_USER')][warehouse] ) }}
{%- endmacro %}

however when i do dbt run i get these weird error:
'MacroManifest' object has no attribute 'env_vars'

How could I read the current_user inside this macro?

For what I can tell, it seems that you cant read env_vars from inside this macro.

For example, this macro definition throws the exception

{% macro snowflake_warehouse(warehouse) -%}
  {% set foo = env_var('SNOWFLAKE_USER') %}
  {{ return(warehouse) }}
{%- endmacro %}

while this one doesnt

{% macro snowflake_warehouse(warehouse) -%}
  {{ return(warehouse) }}
{%- endmacro %}

UPDATE: After a lot of help from dylan at the dbt slack channel, this particular use case (using the snowflake user to decide which warehouse to run) can be solved by using target.user.

However, there seem to be a bug with the implementation that makes it impossible to reference environment variables in this particular macro

@McKnight-42 McKnight-42 mentioned this pull request Mar 23, 2023
6 tasks
McKnight-42 added a commit that referenced this pull request Mar 23, 2023
@McKnight-42
Copy link
Contributor Author

McKnight-42 commented Mar 24, 2023

after some more digging we have decided for now to actually revert #503 we now think that this method is the wrong way as it allows for some unintended consequences and will be trying to find a better way forward. sorry for any inconvenience this causes

McKnight-42 added a commit that referenced this pull request Mar 24, 2023
* reverting #505

* Revert "added macro for wh selection (#503)"

This reverts commit 2e061fc.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CT-2011] [PR Review] added macro for wh selection #115
4 participants