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

Quote databases properly (#1396) #1402

Merged
merged 6 commits into from
Apr 30, 2019
Merged

Conversation

beckjake
Copy link
Contributor

@beckjake beckjake commented Apr 16, 2019

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 like DBT and "dbt". I left that in, because the old way was wrong.

@beckjake beckjake force-pushed the fix/quote-databases-properly branch 2 times, most recently from d5d0e2e to 68febd0 Compare April 16, 2019 19:09
@beckjake beckjake force-pushed the fix/quote-databases-properly branch from 68febd0 to fc4fc57 Compare April 16, 2019 19:51
@cmcarthur cmcarthur requested a review from drewbanin April 18, 2019 15:03
@drewbanin
Copy link
Contributor

I don't think this solves the full problem here:

  • In list_relations we create an information_schema Relation
  • In the information_schema method, we use self.quote_policy to determine if we should quote the database
  • self.quote_policy['database'] is False on Snowflake regardless of how quoting is configured in dbt_project.yml

I determined this in two ways:

  1. I threw a debugger in here and checked out the value of self.quote_policy
  2. I tried to run the following model against a Snowflake target with a database called 1234-test-abc:
select 1

{% do adapter.already_exists(this.schema, this.table) %}

Regardless of the quoting config specified in dbt_project.yml, dbt will not quote the database name when querying the information schema tables. I think we apply these quoting configs after parsing, as the code above fails, but the code below works in this branch:

select 1
{% if execute %}
    {% do adapter.already_exists(this.schema, this.table) %}
{% endif %}

Jacob Beck added 2 commits April 22, 2019 11:29
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
@beckjake beckjake force-pushed the fix/quote-databases-properly branch 2 times, most recently from b629a5e to 8b58b20 Compare April 22, 2019 18:29
Copy link
Contributor

@drewbanin drewbanin left a 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('"'):
Copy link
Contributor

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() + '-' %}
Copy link
Contributor

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'),
Copy link
Contributor

Choose a reason for hiding this comment

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

@beckjake beckjake merged commit 5a3e3ba into dev/0.13.1 Apr 30, 2019
@beckjake beckjake deleted the fix/quote-databases-properly branch April 30, 2019 16:15
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.

2 participants