From 7dea1458ba4a63bafa2f4188768f1059b09dabdd Mon Sep 17 00:00:00 2001 From: Mila Page <67295367+VersusFacit@users.noreply.github.com> Date: Tue, 8 Oct 2024 15:35:08 -0700 Subject: [PATCH] Fix Dynamic Iceberg Table Required DDL Params (#1201) * Fix base location not rendering without subpath and add tests. Take optional off params that are not optional in dynamic table create DDL. * Add changelog. * Revert changes to external volume * revert changes to catalog optionality. * Tabs. * Fix base_location_subpath generation for dynamic tables. --------- Co-authored-by: VersusFacit --- .../unreleased/Fixes-20241008-122635.yaml | 6 ++ .../snowflake/relation_configs/catalog.py | 8 +- .../macros/relations/dynamic_table/create.sql | 2 +- .../relations/dynamic_table/replace.sql | 2 +- tests/functional/iceberg/models.py | 85 +++++++++++++++++++ tests/functional/iceberg/test_table_basic.py | 72 +++------------- 6 files changed, 111 insertions(+), 64 deletions(-) create mode 100644 .changes/unreleased/Fixes-20241008-122635.yaml create mode 100644 tests/functional/iceberg/models.py diff --git a/.changes/unreleased/Fixes-20241008-122635.yaml b/.changes/unreleased/Fixes-20241008-122635.yaml new file mode 100644 index 000000000..c069283d6 --- /dev/null +++ b/.changes/unreleased/Fixes-20241008-122635.yaml @@ -0,0 +1,6 @@ +kind: Fixes +body: Dynamic Iceberg table base_location_subpath generation fix. +time: 2024-10-08T12:26:35.521308-07:00 +custom: + Author: versusfacit + Issue: "1200" diff --git a/dbt/adapters/snowflake/relation_configs/catalog.py b/dbt/adapters/snowflake/relation_configs/catalog.py index 09e338635..c8d7de40f 100644 --- a/dbt/adapters/snowflake/relation_configs/catalog.py +++ b/dbt/adapters/snowflake/relation_configs/catalog.py @@ -1,5 +1,5 @@ from dataclasses import dataclass -from typing import Any, Dict, Optional, TYPE_CHECKING, Set +from typing import Any, Dict, Optional, TYPE_CHECKING, Set, List if TYPE_CHECKING: import agate @@ -82,8 +82,10 @@ def parse_relation_config(cls, relation_config: RelationConfig) -> Dict[str, Any if external_volume := relation_config.config.extra.get("external_volume"): config_dict["external_volume"] = external_volume - if base_location := relation_config.config.extra.get("base_location_subpath"): - config_dict["base_location"] = base_location + catalog_dirs: List[str] = ["_dbt", relation_config.schema, relation_config.name] + if base_location_subpath := relation_config.config.extra.get("base_location_subpath"): + catalog_dirs.append(base_location_subpath) + config_dict["base_location"] = "/".join(catalog_dirs) return config_dict diff --git a/dbt/include/snowflake/macros/relations/dynamic_table/create.sql b/dbt/include/snowflake/macros/relations/dynamic_table/create.sql index 0bd190dcc..4ebcf145b 100644 --- a/dbt/include/snowflake/macros/relations/dynamic_table/create.sql +++ b/dbt/include/snowflake/macros/relations/dynamic_table/create.sql @@ -75,7 +75,7 @@ warehouse = {{ dynamic_table.snowflake_warehouse }} {{ optional('external_volume', dynamic_table.catalog.external_volume) }} {{ optional('catalog', dynamic_table.catalog.name) }} - base_location = {{ dynamic_table.catalog.base_location }} + base_location = '{{ dynamic_table.catalog.base_location }}' {{ optional('refresh_mode', dynamic_table.refresh_mode) }} {{ optional('initialize', dynamic_table.initialize) }} as ( diff --git a/dbt/include/snowflake/macros/relations/dynamic_table/replace.sql b/dbt/include/snowflake/macros/relations/dynamic_table/replace.sql index f9ba1275a..2e7b4566a 100644 --- a/dbt/include/snowflake/macros/relations/dynamic_table/replace.sql +++ b/dbt/include/snowflake/macros/relations/dynamic_table/replace.sql @@ -74,7 +74,7 @@ warehouse = {{ dynamic_table.snowflake_warehouse }} {{ optional('external_volume', dynamic_table.catalog.external_volume) }} {{ optional('catalog', dynamic_table.catalog.name) }} - base_location = {{ dynamic_table.catalog.base_location }} + base_location = '{{ dynamic_table.catalog.base_location }}' {{ optional('refresh_mode', dynamic_table.refresh_mode) }} {{ optional('initialize', dynamic_table.initialize) }} as ( diff --git a/tests/functional/iceberg/models.py b/tests/functional/iceberg/models.py new file mode 100644 index 000000000..6433f74bf --- /dev/null +++ b/tests/functional/iceberg/models.py @@ -0,0 +1,85 @@ +_MODEL_BASIC_TABLE_MODEL = """ +{{ + config( + materialized = "table", + cluster_by=['id'], + ) +}} +select 1 as id +""" + +_MODEL_BASIC_ICEBERG_MODEL = """ +{{ + config( + transient = "true", + materialized = "table", + cluster_by=['id'], + table_format="iceberg", + external_volume="s3_iceberg_snow", + base_location_subpath="subpath", + ) +}} + +select * from {{ ref('first_table') }} +""" + +_MODEL_BASIC_DYNAMIC_TABLE_MODEL = """ +{{ config( + materialized='dynamic_table', + snowflake_warehouse='DBT_TESTING', + target_lag='1 minute', + refresh_mode='INCREMENTAL', + table_format='iceberg', + external_volume='s3_iceberg_snow', +) }} + +select * from {{ ref('first_table') }} +""" + +_MODEL_BASIC_DYNAMIC_TABLE_MODEL_WITH_SUBPATH = """ +{{ config( + materialized='dynamic_table', + snowflake_warehouse='DBT_TESTING', + target_lag='1 minute', + refresh_mode='INCREMENTAL', + table_format='iceberg', + external_volume='s3_iceberg_snow', + base_location_subpath='subpath', +) }} + +select * from {{ ref('first_table') }} +""" + +_MODEL_BUILT_ON_ICEBERG_TABLE = """ +{{ + config( + materialized = "table", + ) +}} +select * from {{ ref('iceberg_table') }} +""" + +_MODEL_TABLE_BEFORE_SWAP = """ +{{ + config( + materialized = "table", + ) +}} +select 1 as id +""" + +_MODEL_VIEW_BEFORE_SWAP = """ +select 1 as id +""" + +_MODEL_TABLE_FOR_SWAP_ICEBERG = """ +{{ + config( + materialized = "table", + table_format="iceberg", + external_volume="s3_iceberg_snow", + base_location_subpath="subpath", + ) +}} +select 1 as id +""" diff --git a/tests/functional/iceberg/test_table_basic.py b/tests/functional/iceberg/test_table_basic.py index 0bfdf59f1..e835a5fce 100644 --- a/tests/functional/iceberg/test_table_basic.py +++ b/tests/functional/iceberg/test_table_basic.py @@ -4,64 +4,16 @@ from dbt.tests.util import run_dbt, rm_file, write_file -_MODEL_BASIC_TABLE_MODEL = """ -{{ - config( - materialized = "table", - cluster_by=['id'], - ) -}} -select 1 as id -""" - -_MODEL_BASIC_ICEBERG_MODEL = """ -{{ - config( - transient = "true", - materialized = "table", - cluster_by=['id'], - table_format="iceberg", - external_volume="s3_iceberg_snow", - base_location_subpath="subpath", - ) -}} - -select * from {{ ref('first_table') }} -""" - -_MODEL_BUILT_ON_ICEBERG_TABLE = """ -{{ - config( - materialized = "table", - ) -}} -select * from {{ ref('iceberg_table') }} -""" - -_MODEL_TABLE_BEFORE_SWAP = """ -{{ - config( - materialized = "table", - ) -}} -select 1 as id -""" - -_MODEL_VIEW_BEFORE_SWAP = """ -select 1 as id -""" - -_MODEL_TABLE_FOR_SWAP_ICEBERG = """ -{{ - config( - materialized = "table", - table_format="iceberg", - external_volume="s3_iceberg_snow", - base_location_subpath="subpath", - ) -}} -select 1 as id -""" +from tests.functional.iceberg.models import ( + _MODEL_BASIC_TABLE_MODEL, + _MODEL_BASIC_ICEBERG_MODEL, + _MODEL_BASIC_DYNAMIC_TABLE_MODEL, + _MODEL_BASIC_DYNAMIC_TABLE_MODEL_WITH_SUBPATH, + _MODEL_BUILT_ON_ICEBERG_TABLE, + _MODEL_TABLE_BEFORE_SWAP, + _MODEL_VIEW_BEFORE_SWAP, + _MODEL_TABLE_FOR_SWAP_ICEBERG, +) class TestIcebergTableBuilds: @@ -75,11 +27,13 @@ def models(self): "first_table.sql": _MODEL_BASIC_TABLE_MODEL, "iceberg_table.sql": _MODEL_BASIC_ICEBERG_MODEL, "table_built_on_iceberg_table.sql": _MODEL_BUILT_ON_ICEBERG_TABLE, + "dynamic_table.sql": _MODEL_BASIC_DYNAMIC_TABLE_MODEL, + "dynamic_tableb.sql": _MODEL_BASIC_DYNAMIC_TABLE_MODEL_WITH_SUBPATH, } def test_iceberg_tables_build_and_can_be_referred(self, project): run_results = run_dbt() - assert len(run_results) == 3 + assert len(run_results) == 5 class TestIcebergTableTypeBuildsOnExistingTable: