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 quote_identifiers argument #135

Merged
merged 2 commits into from
May 13, 2019
Merged

Added quote_identifiers argument #135

merged 2 commits into from
May 13, 2019

Conversation

carlyfk
Copy link

@carlyfk carlyfk commented May 8, 2019

Indicates whether to surround column aliases with double quotes or not.

Indicates whether to surround column aliases with double quotes or not.
@drewbanin
Copy link
Contributor

Thanks for making this PR @carlyfk! Can you tell me more about the problem you had which was solved by this PR? It looks to me like a Snowflake column caps/quoting thing, right?

dbt-utils doesn't do a great job of consistently quoting columns/tables/schemas/databases in general, and I'm super interested in trying to fix that! I think PRs like this are a good idea, but I'm also interested in thinking about a more general approach to solving this type of problem for every macro in here!

Let me know about the use case (for my edification). Can you also update the README.md to document this new argument? I can see about that test failure -- it's something on our end and not your code I think :)

@carlyfk
Copy link
Author

carlyfk commented May 10, 2019

@drewbanin Thanks for your feedback! I think the issue was that one of my columns being generated is called NAME. The macro then outputs the SQL and NAME becomes "NAME". And then the rest of my downstream code fails because I'm referring to the column by NAME and not "NAME". By removing the forced quotes in the macro, I don't have to change any of my references downstream. Btw, this only became an issue with Snowflake and was not an issue in Redshift.

I'll update the README.md document as well. :)

As a side note, this is my first PR and I'm a relative beginner in git. I've managed to completely jack up my codebase as a result and am not sure how to deal with the whole submodule concept. Any recs for blogs I can read on how to do this cleanly?

@drewbanin
Copy link
Contributor

ok, thanks for the additional context!

this is my first PR and I'm a relative beginner in git

That's amazing - this is super well done and I assumed this wasn't your first time around the block. Nice work!

I've managed to completely jack up my codebase as a result and am not sure how to deal with the whole submodule concept

Hmmm, submodules are definitely painful, but you shouldn't need one here! Did you clone dbt-utils into your dbt_modules directory? Instead, you should be able to use a local package for development. Check out an example here. We could definitely do a better job of documenting this type of workflow! Let me know if that doesn't fix this for you -- happy to help!

I just kicked off the tests here - are you ready to merge this when they pass?

@carlyfk
Copy link
Author

carlyfk commented May 13, 2019

Sorry, I'm not getting email notifications when you reply so I no idea. Let me know I need to do to merge!

@drewbanin drewbanin merged commit 1b983ed into dbt-labs:master May 13, 2019
@drewbanin
Copy link
Contributor

This is merged! We'll cut a new release, v0.2.0, which includes this change (and some others) in the coming days!

Thanks for your contribution to dbt-utils!!

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.

None yet

2 participants