-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Quote databases properly (#1396) #1402
Conversation
d5d0e2e
to
68febd0
Compare
Fix a copy+paste error that broke database quoting configuration
68febd0
to
fc4fc57
Compare
I don't think this solves the full problem here:
I determined this in two ways:
Regardless of the quoting config specified in
|
Add an already_exists call to a test that requires an alternative database Make the alternative database on snowlfake be one that must be quoted
b629a5e
to
8b58b20
Compare
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.
LGTM!
@tayloramurphy are you able to try installing from this branch and testing out your use case? The code here looks good, and I gave it a spin locally too. If you're able to test it out, I'd feel really good about merging this in
@@ -25,7 +25,10 @@ def date_function(cls): | |||
|
|||
@available_raw | |||
def verify_database(self, database): | |||
database = database.strip('"') | |||
if database.startswith('"'): |
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.
This isn't the most pleasant thing in the world, but I think it's a good and appropriate fix for this problem. Nice
{%- set tgt = ref('seed') -%} | ||
{%- set got = adapter.get_relation(database=tgt.database, schema=tgt.schema, identifier=tgt.identifier) | string -%} | ||
{% set replaced = got.replace('"', '-') %} | ||
{% set expected = "-" + tgt.database.upper() + '-.-' + tgt.schema.upper() + '-.-' + tgt.identifier.upper() + '-' %} |
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.
-.-
is exactly how i feel about Snowflake quoting
'account': os.getenv('SNOWFLAKE_TEST_ACCOUNT'), | ||
'user': os.getenv('SNOWFLAKE_TEST_USER'), | ||
'password': os.getenv('SNOWFLAKE_TEST_PASSWORD'), | ||
'database': os.getenv('SNOWFLAKE_TEST_QUOTED_DATABASE'), |
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.
Can you update https://github.com/fishtown-analytics/dbt/blob/dev/wilt-chamberlain/test.env.sample accordingly?
Fixes #1396
There are two bugs here: one is around lowercasing/uppercasing on quoting, and one is around not setting the database quote policy to true when listing relations. The latter is the issue reported in 1396, and I have a test for it that is... okay. I can't come up with a test for the former that doesn't involve creating a snowflake test database that is either lowercase or starts with a number.
Sadly, even though the bug is in the common base and sql adapters, all tests have to be against snowflake because only snowflake defaults to quoting = false.
While I was trying to get the tests running on postgres, I had to tweak
PostgresAdapter.verify_database
- we used to reject perfectly valid mixing of stuff likeDBT
and"dbt"
. I left that in, because the old way was wrong.