-
Notifications
You must be signed in to change notification settings - Fork 41
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
fixes problems with quoting: true #139
Conversation
@@ -1,6 +1,6 @@ | |||
{% macro create_merge_objects_udf(relation) %} | |||
|
|||
create or replace function {{ relation.database }}.{{ relation.schema }}.merge_objects(obj1 variant, obj2 variant) | |||
create or replace function {{ adapter.quote_as_configured(this.database, 'database') }}.{{ adapter.quote_as_configured(this.schema, 'schema') }}.merge_objects(obj1 variant, obj2 variant) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@NiallRees can you have a look at this?
thanks for opening the PR @ernestoongaro and apologies for the delays here! will hopefuly get this merged in tomorrow. thanks for your patience! |
No problem @ian-whitestone ! I'm having a customer test this on a huge codebase with quoting on schema/identifier/database so I'm glad it's not merged yet as I want to make sure we catch all problems. Would love to add a test configuration that tests quoting on & off. It's unfortunately a thing people do :( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving but realise you still might make some more changes, so let me know when you're finished!
thanks @NiallRees that's amazing. Finished doing a "production" run and everything is working now with the last 2 commits. Any thoughts on adding an automated test with these settings? Though it's rare, people seem to want to quote identifiers from time to time
|
So had a chat to @NiallRees about longterm support for aliases: Another option was to use import CTEs which have implicit casting but the option we went for today was to leave it alone and just merge this. (1)
|
Quite similar to #52 (which was fixed) it looks like there is a few more places where tables need to get aliased.
As part of this would like to also see about adding a test configuration where we try this setting on every PR, or at least add it to a linting rule