search

dbt-labs / dbt-external-tables

dbt macros to stage external sources
https://hub.getdbt.com/dbt-labs/dbt_external_tables/latest/
Apache License 2.0
294 stars 119 forks source link

Add source node database #196

Closed epapineau closed 1 year ago

epapineau commented 1 year ago

Description & motivation

Resolves #195 - please let me know if this should be moved to a snowflake specific macro

Checklist

guillesd commented 1 year ago

HI @epapineau good to see that you are improving on my changes! We indeed saw that if your target.database is different from the source database, then this will create the schema in your target instead of where you want this!

For BigQuery your tests are failing since project ids in GCP usually contain dashes "-", which are not allowed in SQL. The fix for this is to put the project id around back ticks (`), so `project-id`.schema. If this is allowed by other SQL dialects (Databricks, Snowflake, REdshift...) then you can add this change to the common macro (the one that you changed for this PR). If this doesn't work for other adapters, then you should add a specific macro for BigQuery under plugins/bigquery/.

Hope this helps! Cheers

jeremyyeo commented 1 year ago

Thanks for chiming in @guillesd - yeah indeed the - on BQ project/database is throwing this one off. I think we could get away with doing something like:

https://github.com/dbt-labs/dbt-external-tables/compare/main...experiment/alt-fix-create-with-db

Separating out the schema fqn into it's own macro so that we can quote the database/project specifically for BQ.

The full blown fix would be to strip out the various direct "create schema if not exist ... " DDLs and replace them with calls to adapter.create_schema (https://docs.getdbt.com/reference/dbt-jinja-functions/adapter#create_schema) but I think the quick fix approach above should suffice for now.

epapineau commented 1 year ago

Thanks for the feedback y'all~💟 I'll have the suggestions integrated by next week.

epapineau commented 1 year ago

@jeremyyeo & @guillesd Updated & passing for bigquery~ We've still got a redshift test failing, but AFAICT it looks like the test configuration more than the contents of this PR.