-
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
Fix/jeb snowflake source quoting #1338
Fix/jeb snowflake source quoting #1338
Conversation
- do not quote snowflake database identifiers by default - do not find relations in source schemas in list_relations - do not render database names in stdout if a custom database is not specified
…2' into fix/jeb-snowflake-source-quoting
core/dbt/adapters/base/impl.py
Outdated
""" | ||
info_schema_name_map = SchemaSearchMap() | ||
for node in manifest.nodes.values(): | ||
if node.resource_type in NodeType.executable(): |
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.
i think we want to include all nodes (or at least, add Sources) here. As a result of this code, i think the get_catalog
macro doesn't correctly loop over databases for sources!
I made a change over here (https://github.com/fishtown-analytics/dbt/pull/1332/files#diff-f6971115b03b21cf1f245c037cda4978R239) which only populated the cache for schemas that dbt was going to try to build resouces into. The single place we need to loop over Source databases is in the get_catalog
macro, as we use it to build Sources into the docs site. Otherwise, dbt shouldn't care about the databases provided in Sources.
Edit: words
Edit2: I tested this out locally after removing if node.resource_type...
and it worked great! We also need to use create_from
instead of create_from_node
I believe
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.
Approved pending one comment on an error message!
core/dbt/adapters/base/impl.py
Outdated
seen = {r.database.lower() for r in self if r.database} | ||
if len(seen) > 1: | ||
dbt.exceptions.raise_compiler_error( | ||
'flatten() requires <=1 database (got {})'.format(seen) |
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.
It looks like this is only used in the pg/redshift version of _get_cache_schemas
. Can we catch this error and re-raise something pg/redshift-specific? As it stands, I think users would have a hard time knowing what this error means. Alternatively, we can just call search()
from the pg's _get_cache_schemas
and validate for dupes there.
I'd like to show an error message like:
Cross-db references not allowed in {adapter} ({db1} vs {db2})
I don't feel strongly about the exact wording or location of the exception, but would prefer that it says something like "your database won't let you do this".
The other option is to just call search()
in pg/redshift and let some downstream SQL handle this. Either something we write (like our get_catalog()
pg implementation which does not allow multiple dbs), or the database will reject the query with a more contextful error message. Lmk what you think
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.
I think my latest commit resolves this, it does something like: Cross-db references not allowed in {adapter}: Got {seen}
. Does that seem ok?
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.
perfect 👍
the new error message looks great -- let's let the tests run then ship this in the AM! |
Fixes #1317
Fixes the other bug described in #1326
HEAD also includes the PR for #1332 , with merge conflicts resolved and the snowflake docs generate tests fixed
let's see if windows works?