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

fix: session db/schema and macro references #25

Merged
merged 1 commit into from
Jan 18, 2023
Merged

fix: session db/schema and macro references #25

merged 1 commit into from
Jan 18, 2023

Conversation

calleo
Copy link
Contributor

@calleo calleo commented Nov 30, 2022

Hi,

I could not get it to work neither using DBT 1.2 or 1.3. Calls that are made to macros within the package fail and tags are being looked for and created in the database and schema specified in the profile, not the database/schema of the model.

With the changes suggested by this PR I got it to work, by:

  • Adding USE DATABASE .. and USE SCHEMA ... in the main macro. This could also be solved by using the fully qualified schema and table names everywhere but this solution seemed easier. This will also mean that tags are created in the database/schema of the model. Makes sense to me, but I am not sure what the expected behavior was? If your DBT setup creates all models in the same database, this won't be an issue.

  • Prefixed the macro calls with snowflake_utils. This fixed the issues I was having, but not sure why the prefix is needed from within the package.

  • Also had issues with the initial run, since the macro didn't return anything and DBT tried to execute it, which rendered an error (fixed by returning an empty string):

Database Error
  001003 (42000): SQL compilation error:
  syntax error line 1 at position 0 unexpected 'None'.

Related to #24

But TBH, I don't really understand why I am facing these issues seeing that #21 has been merged and things seem to work fine at that point.

@jamesweakley
Copy link
Contributor

Thanks @calleo for the PR. I'm not sure why you sometimes need to qualify the macro calls, certainly in my past experience it was optional within a package.

The only part of the PR I'm unsure about is changing the location of tags specifically to match the model.
I think it's OK for existing users, because the macro was already checking the fully qualified location for existing tags.

@jamesweakley
Copy link
Contributor

If people do prefer a fixed location for all their tags, we can support that in future via some new environment variables

@JFrackson JFrackson self-requested a review January 10, 2023 13:22
Copy link
Contributor

@JFrackson JFrackson left a comment

Choose a reason for hiding this comment

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

This looks like a good solution, but I agree with @jamesweakley that we may want to think about exposing these as environment variables in the future if there's a divergent use case. I can make the issue for that after merging this, but before doing that do you mind updating the README to be a bit more specific about where the tags will be written to? Just a sentence would go a long way.

* Need to set current database and schema for Snowflake to create
tags in the correct place and look for tags in the correct place

* When calling macros from the packaged, they need to be prefixed
with the package name for DBT to find them

* Add note about tag location in readme
@calleo
Copy link
Contributor Author

calleo commented Jan 12, 2023

@JFrackson thank you for taking a look at the PR! I have added a few lines to the readme to clarify where the tags will be created.

Copy link
Contributor

@JFrackson JFrackson left a comment

Choose a reason for hiding this comment

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

lgtm, thanks for the contribution! Once this is merged I'll add the issue for updating to make the destination configurable as an env var

@calleo
Copy link
Contributor Author

calleo commented Jan 17, 2023

No worries! Thanks for putting this together. I don't seem to be bale to merge so that's up to one of the maintainers.

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.

3 participants