Use 3-level namespace when catalog is set. #94
Conversation
ueshin
changed the title
|
thrilled to see work in this direction, I was just looking for this feature 3 days ago and happy to see its already in a PR. Is there anything I can do to help? Thanks :) |
ueshin
changed the title
ueshin
changed the title
|
Let me separate some changes to another PR. |
|
Now this is based on #98. |
| @@ -67,3 +80,113 @@ def execute( | |||
| finally: | |||
| if staging_table is not None: | |||
| self.drop_relation(staging_table) | |||
|
|
|||
| def list_relations_without_caching( | |||
There was a problem hiding this comment.
It would be nice to add a new method in dbt-spark that returns the catalog field to avoid duplicate code in the future (but we need to know the release schedule for dbt-spark).
There was a problem hiding this comment.
Yes, we can ask dbt-spark to make the change.
Let's have them here and work on it in a separate PR for now.
| {% set tmp_relation = base_relation.incorporate(path = { | ||
| "identifier": tmp_identifier, | ||
| "schema": None, | ||
| "database": None |
There was a problem hiding this comment.
If we don't specify "database": None here, what's the default value for it?
There was a problem hiding this comment.
The tmp_relation will hold the database of base_relation, that results with database.identifier for the temp relation name if base_relation has database field.
There was a problem hiding this comment.
Make sense. Thanks for the explanation!
| @@ -58,7 +73,7 @@ | |||
|
|
|||
| {{ adapter.valid_snapshot_target(target_relation) }} | |||
|
|
|||
| {% set staging_table = spark_build_snapshot_staging_table(strategy, sql, target_relation) %} | |||
| {% set staging_table = databricks_build_snapshot_staging_table(strategy, sql, target_relation) %} | |||
There was a problem hiding this comment.
Not urgent but should we use the dispatch pattern here to make it backward compatible?
There was a problem hiding this comment.
That's a good point.
Actually I think if users override spark_build_snapshot_staging_table macro, it will highly likely break the catalog support.
How about using spark_build_snapshot_staging_table only when target_relation.database is None?
Also maybe we should mention it in the change log and the release note.
| # Insert a row into the seed model with an invalid id. | ||
| self.run_sql_file("insert_invalid_id.sql") | ||
| self.run_and_check_failure( | ||
| model_name, | ||
| err_msg="CHECK constraint id_greater_than_zero", | ||
| ) | ||
| self.check_staging_table_cleaned() | ||
| self.run_sql(f"delete from {schema}.seed where id = 0") | ||
| self.run_sql("delete from {database_schema}.seed where id = 0") |
There was a problem hiding this comment.
Interesting, so if we use {database_schema} in the query then dbt will automatically change it to use the target database and target schema?
There was a problem hiding this comment.
Actually I made some changes in tests/integration/base.py to handle database_schema.
There was a problem hiding this comment.
👍 it's in transform_sql
| {% set tmp_relation = base_relation.incorporate(path = { | ||
| "identifier": tmp_identifier, | ||
| "schema": None, | ||
| "database": None |
There was a problem hiding this comment.
Make sense. Thanks for the explanation!
| # Insert a row into the seed model with an invalid id. | ||
| self.run_sql_file("insert_invalid_id.sql") | ||
| self.run_and_check_failure( | ||
| model_name, | ||
| err_msg="CHECK constraint id_greater_than_zero", | ||
| ) | ||
| self.check_staging_table_cleaned() | ||
| self.run_sql(f"delete from {schema}.seed where id = 0") | ||
| self.run_sql("delete from {database_schema}.seed where id = 0") |
There was a problem hiding this comment.
👍 it's in transform_sql
|
Thanks! merging. |
### Description Uses 3-level namespace when catalog is set.
### Description Uses 3-level namespace when catalog is set.
Description
Uses 3-level namespace when catalog is set.