diff --git a/.changes/unreleased/Fixes-20241104-104610.yaml b/.changes/unreleased/Fixes-20241104-104610.yaml new file mode 100644 index 000000000..c512d0bdd --- /dev/null +++ b/.changes/unreleased/Fixes-20241104-104610.yaml @@ -0,0 +1,7 @@ +kind: Fixes +body: 'Performance fixes for snowflake microbatch strategy: use temp view instead + of table, remove unnecessary ''using'' clause' +time: 2024-11-04T10:46:10.005317-05:00 +custom: + Author: michelleark + Issue: "1228" diff --git a/.changes/unreleased/Fixes-20241104-172349.yaml b/.changes/unreleased/Fixes-20241104-172349.yaml new file mode 100644 index 000000000..07c90d93c --- /dev/null +++ b/.changes/unreleased/Fixes-20241104-172349.yaml @@ -0,0 +1,6 @@ +kind: Fixes +body: Iceberg quoting ignore fix. +time: 2024-11-04T17:23:49.706297-08:00 +custom: + Author: versusfacit + Issue: "1227" diff --git a/.changes/unreleased/Under the Hood-20241016-035544.yaml b/.changes/unreleased/Under the Hood-20241016-035544.yaml new file mode 100644 index 000000000..59e4f70de --- /dev/null +++ b/.changes/unreleased/Under the Hood-20241016-035544.yaml @@ -0,0 +1,6 @@ +kind: Under the Hood +body: Add telemetry function +time: 2024-10-16T03:55:44.144174-07:00 +custom: + Author: versusfacit + Issue: "301" diff --git a/.changes/unreleased/Under the Hood-20241106-113249.yaml b/.changes/unreleased/Under the Hood-20241106-113249.yaml new file mode 100644 index 000000000..0437a8c88 --- /dev/null +++ b/.changes/unreleased/Under the Hood-20241106-113249.yaml @@ -0,0 +1,6 @@ +kind: Under the Hood +body: remove SnowflakeAdapterResponse in favor of updated AdapterResponse in base +time: 2024-11-06T11:32:49.503467-08:00 +custom: + Author: colin-rogers-dbt + Issue: "1233" diff --git a/.github/scripts/integration-test-matrix.js b/.github/scripts/integration-test-matrix.js index 756c21d5e..e2c88b00b 100644 --- a/.github/scripts/integration-test-matrix.js +++ b/.github/scripts/integration-test-matrix.js @@ -44,7 +44,7 @@ module.exports = ({ context }) => { if (labels.includes("test macos") || testAllLabel) { include.push({ - os: "macos-12", + os: "macos-14", adapter, "python-version": pythonVersion, }); @@ -78,7 +78,7 @@ module.exports = ({ context }) => { // additionally include runs for all adapters, on macos and windows, // but only for the default python version for (const adapter of supportedAdapters) { - for (const operatingSystem of ["windows-latest", "macos-12"]) { + for (const operatingSystem of ["windows-latest", "macos-14"]) { include.push({ os: operatingSystem, adapter: adapter, diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 4913917f4..24d2fa60b 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -174,7 +174,7 @@ jobs: strategy: fail-fast: false matrix: - os: [ubuntu-latest, macos-12, windows-latest] + os: [ubuntu-latest, macos-14, windows-latest] python-version: ['3.9', '3.10', '3.11', '3.12'] dist-type: ['whl', 'gz'] diff --git a/dbt/adapters/snowflake/connections.py b/dbt/adapters/snowflake/connections.py index 10bee30f0..fc2c09c19 100644 --- a/dbt/adapters/snowflake/connections.py +++ b/dbt/adapters/snowflake/connections.py @@ -84,11 +84,6 @@ def snowflake_private_key(private_key: RSAPrivateKey) -> bytes: ) -@dataclass -class SnowflakeAdapterResponse(AdapterResponse): - query_id: str = "" - - @dataclass class SnowflakeCredentials(Credentials): account: str @@ -447,17 +442,17 @@ def cancel(self, connection): logger.debug("Cancel query '{}': {}".format(connection_name, res)) @classmethod - def get_response(cls, cursor) -> SnowflakeAdapterResponse: + def get_response(cls, cursor) -> AdapterResponse: code = cursor.sqlstate if code is None: code = "SUCCESS" - - return SnowflakeAdapterResponse( + query_id = str(cursor.sfqid) if cursor.sfqid is not None else None + return AdapterResponse( _message="{} {}".format(code, cursor.rowcount), rows_affected=cursor.rowcount, code=code, - query_id=cursor.sfqid, + query_id=query_id, ) # disable transactional logic by default on Snowflake diff --git a/dbt/adapters/snowflake/impl.py b/dbt/adapters/snowflake/impl.py index 6320893e1..dc0926d93 100644 --- a/dbt/adapters/snowflake/impl.py +++ b/dbt/adapters/snowflake/impl.py @@ -4,6 +4,7 @@ from dbt.adapters.base.impl import AdapterConfig, ConstraintSupport from dbt.adapters.base.meta import available from dbt.adapters.capability import CapabilityDict, CapabilitySupport, Support, Capability +from dbt.adapters.contracts.relation import RelationConfig from dbt.adapters.sql import SQLAdapter from dbt.adapters.sql.impl import ( LIST_SCHEMAS_MACRO_NAME, @@ -25,6 +26,7 @@ SnowflakeRelationType, TableFormat, ) + from dbt.adapters.snowflake import SnowflakeColumn from dbt.adapters.snowflake import SnowflakeConnectionManager from dbt.adapters.snowflake import SnowflakeRelation @@ -261,6 +263,11 @@ def list_relations_without_caching( # this can be collapsed once Snowflake adds is_iceberg to show objects columns = ["database_name", "schema_name", "name", "kind", "is_dynamic"] if self.behavior.enable_iceberg_materializations.no_warn: + # The QUOTED_IDENTIFIERS_IGNORE_CASE setting impacts column names like + # is_iceberg which is created by dbt, but it does not affect the case + # of column values in Snowflake's SHOW OBJECTS query! This + # normalization step ensures metadata queries are handled consistently. + schema_objects = schema_objects.rename(column_names={"IS_ICEBERG": "is_iceberg"}) columns.append("is_iceberg") return [self._parse_list_relations_result(obj) for obj in schema_objects.select(columns)] @@ -419,3 +426,18 @@ def valid_incremental_strategies(self): def debug_query(self): """Override for DebugTask method""" self.execute("select 1 as id") + + @classmethod + def _get_adapter_specific_run_info(cls, config: RelationConfig) -> Dict[str, Any]: + table_format: Optional[str] = None + if ( + config + and hasattr(config, "_extra") + and (relation_format := config._extra.get("table_format")) + ): + table_format = relation_format + + return { + "adapter_type": "snowflake", + "table_format": table_format, + } diff --git a/dbt/include/snowflake/macros/adapters.sql b/dbt/include/snowflake/macros/adapters.sql index b60cea0b0..0ca756c6c 100644 --- a/dbt/include/snowflake/macros/adapters.sql +++ b/dbt/include/snowflake/macros/adapters.sql @@ -135,7 +135,6 @@ {% endmacro %} {% macro snowflake__list_relations_without_caching(schema_relation, max_iter=10, max_results_per_iter=10000) %} - {%- set max_total_results = max_results_per_iter * max_iter -%} {%- set sql -%} {% if schema_relation is string %} @@ -147,7 +146,7 @@ {# -- Gated for performance reason. If you don't want Iceberg, you shouldn't pay the -- latency penalty. #} {% if adapter.behavior.enable_iceberg_materializations.no_warn %} - select all_objects.*, is_iceberg as "is_iceberg" + select all_objects.*, is_iceberg from table(result_scan(last_query_id(-1))) all_objects left join INFORMATION_SCHEMA.tables as all_tables on all_tables.table_name = all_objects."name" diff --git a/dbt/include/snowflake/macros/materializations/incremental.sql b/dbt/include/snowflake/macros/materializations/incremental.sql index d73525d6d..dbb79de02 100644 --- a/dbt/include/snowflake/macros/materializations/incremental.sql +++ b/dbt/include/snowflake/macros/materializations/incremental.sql @@ -20,7 +20,7 @@ The append strategy can use a view because it will run a single INSERT statement. - When unique_key is none, the delete+insert strategy can use a view beacuse a + When unique_key is none, the delete+insert and microbatch strategies can use a view beacuse a single INSERT statement is run with no DELETES as part of the statement. Otherwise, play it safe by using a temporary table. #} */ @@ -32,10 +32,10 @@ ) %} {% endif %} - {% if strategy == "delete+insert" and tmp_relation_type is not none and tmp_relation_type != "table" and unique_key is not none %} + {% if strategy in ["delete+insert", "microbatch"] and tmp_relation_type is not none and tmp_relation_type != "table" and unique_key is not none %} {% do exceptions.raise_compiler_error( "In order to maintain consistent results when `unique_key` is not none, - the `delete+insert` strategy only supports `table` for `tmp_relation_type` but " + the `" ~ strategy ~ "` strategy only supports `table` for `tmp_relation_type` but " ~ tmp_relation_type ~ " was specified." ) %} @@ -49,7 +49,7 @@ {{ return("view") }} {% elif strategy in ("default", "merge", "append") %} {{ return("view") }} - {% elif strategy == "delete+insert" and unique_key is none %} + {% elif strategy in ["delete+insert", "microbatch"] and unique_key is none %} {{ return("view") }} {% else %} {{ return("table") }} diff --git a/dbt/include/snowflake/macros/materializations/merge.sql b/dbt/include/snowflake/macros/materializations/merge.sql index 57c58afdd..c8ac8d6fd 100644 --- a/dbt/include/snowflake/macros/materializations/merge.sql +++ b/dbt/include/snowflake/macros/materializations/merge.sql @@ -66,7 +66,6 @@ {% do arg_dict.update({'incremental_predicates': incremental_predicates}) %} delete from {{ target }} DBT_INTERNAL_TARGET - using {{ source }} where ( {% for predicate in incremental_predicates %} {%- if not loop.first %}and {% endif -%} {{ predicate }} diff --git a/dbt/include/snowflake/macros/relations/table/create.sql b/dbt/include/snowflake/macros/relations/table/create.sql index e60b93039..e2141df4d 100644 --- a/dbt/include/snowflake/macros/relations/table/create.sql +++ b/dbt/include/snowflake/macros/relations/table/create.sql @@ -1,7 +1,7 @@ {% macro snowflake__create_table_as(temporary, relation, compiled_code, language='sql') -%} {%- if relation.is_iceberg_format and not adapter.behavior.enable_iceberg_materializations.no_warn %} - {% do exceptions.raise_compiler_error('Was unable to create model as Iceberg Table Format. Please set the `enable_iceberg_materializations` behavior flag to True in your dbt_project.yml. For more information, go to .') %} + {% do exceptions.raise_compiler_error('Was unable to create model as Iceberg Table Format. Please set the `enable_iceberg_materializations` behavior flag to True in your dbt_project.yml. For more information, go to https://docs.getdbt.com/reference/resource-configs/snowflake-configs.') %} {%- endif %} {%- set materialization_prefix = relation.get_ddl_prefix_for_create(config.model.config, temporary) -%} diff --git a/tests/functional/adapter/list_relations_tests/test_show_objects.py b/tests/functional/adapter/list_relations_tests/test_show_objects.py index e5eee39d9..91fb94f79 100644 --- a/tests/functional/adapter/list_relations_tests/test_show_objects.py +++ b/tests/functional/adapter/list_relations_tests/test_show_objects.py @@ -3,6 +3,8 @@ import pytest +from pathlib import Path + from dbt.adapters.factory import get_adapter_by_type from dbt.adapters.snowflake import SnowflakeRelation @@ -41,8 +43,32 @@ """ ) +_MODEL_ICEBERG = """ +{{ + config( + materialized = "table", + table_format="iceberg", + external_volume="s3_iceberg_snow", + ) +}} + +select 1 +""" + + +class ShowObjectsBase: + @staticmethod + def list_relations_without_caching(project) -> List[SnowflakeRelation]: + my_adapter = get_adapter_by_type("snowflake") + schema = my_adapter.Relation.create( + database=project.database, schema=project.test_schema, identifier="" + ) + with get_connection(my_adapter): + relations = my_adapter.list_relations_without_caching(schema) + return relations + -class TestShowObjects: +class TestShowObjects(ShowObjectsBase): views: int = 10 tables: int = 10 dynamic_tables: int = 10 @@ -66,16 +92,6 @@ def setup(self, project): run_dbt(["seed"]) run_dbt(["run"]) - @staticmethod - def list_relations_without_caching(project) -> List[SnowflakeRelation]: - my_adapter = get_adapter_by_type("snowflake") - schema = my_adapter.Relation.create( - database=project.database, schema=project.test_schema, identifier="" - ) - with get_connection(my_adapter): - relations = my_adapter.list_relations_without_caching(schema) - return relations - def test_list_relations_without_caching(self, project): relations = self.list_relations_without_caching(project) assert len([relation for relation in relations if relation.is_view]) == self.views @@ -87,3 +103,25 @@ def test_list_relations_without_caching(self, project): len([relation for relation in relations if relation.is_dynamic_table]) == self.dynamic_tables ) + + +class TestShowIcebergObjects(ShowObjectsBase): + @pytest.fixture(scope="class") + def project_config_update(self): + return {"flags": {"enable_iceberg_materializations": True}} + + @pytest.fixture(scope="class") + def models(self): + return {"my_model.sql": _MODEL_ICEBERG} + + def test_quoting_ignore_flag_doesnt_break_iceberg_metadata(self, project): + """https://github.com/dbt-labs/dbt-snowflake/issues/1227 + + The list relations function involves a metadata sub-query. Regardless of + QUOTED_IDENTIFIERS_IGNORE_CASE, this function will fail without proper + normalization within the encapsulating python function after the macro invocation + returns. This test verifies that normalization is working. + """ + run_dbt(["run"]) + + self.list_relations_without_caching(project) diff --git a/tests/functional/generic_test_tests/__init__.py b/tests/functional/generic_test_tests/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/tests/functional/generic_test_tests/_files.py b/tests/functional/generic_test_tests/_files.py new file mode 100644 index 000000000..a9743e43e --- /dev/null +++ b/tests/functional/generic_test_tests/_files.py @@ -0,0 +1,90 @@ +SCHEMA__CONTROL = """ +version: 2 +models: + - name: colors + columns: + - name: color + data_tests: + - not_null +""" + + +SCHEMA__EXPLICIT_WAREHOUSE = """ +version: 2 +models: + - name: colors + columns: + - name: color + data_tests: + - not_null: + config: + snowflake_warehouse: DBT_TESTING_ALT +""" + + +SCHEMA__NOT_NULL = """ +version: 2 +models: + - name: facts + columns: + - name: value + data_tests: + - not_null: + config: + snowflake_warehouse: DBT_TESTING_ALT +""" + + +SCHEMA__RELATIONSHIPS = """ +version: 2 +models: + - name: facts + columns: + - name: color + data_tests: + - relationships: + to: ref('my_colors') + field: color + config: + snowflake_warehouse: DBT_TESTING_ALT +""" + + +SCHEMA__ACCEPTED_VALUES = """ +version: 2 +models: + - name: colors + columns: + - name: color + data_tests: + - accepted_values: + values: ['blue', 'red', 'green'] + config: + snowflake_warehouse: DBT_TESTING_ALT +""" + + +SEED__COLORS = """ +color +blue +green +red +yellow +""".strip() + + +# record 10 is missing a value +# record 7 has a color that's not on COLORS +SEED__FACTS = """ +id,color,value +1,blue,10 +2,red,20 +3,green,30 +4,yellow,40 +5,blue,50 +6,red,60 +7,orange,70 +8,green,80 +9,yellow,90 +10,blue, +""".strip() diff --git a/tests/functional/generic_test_tests/_models.py b/tests/functional/generic_test_tests/_models.py new file mode 100644 index 000000000..e69de29bb diff --git a/tests/functional/generic_test_tests/_schemas.py b/tests/functional/generic_test_tests/_schemas.py new file mode 100644 index 000000000..e69de29bb diff --git a/tests/functional/generic_test_tests/test_generic_tests.py b/tests/functional/generic_test_tests/test_generic_tests.py new file mode 100644 index 000000000..a653a363b --- /dev/null +++ b/tests/functional/generic_test_tests/test_generic_tests.py @@ -0,0 +1,54 @@ +import pytest + +from dbt.tests.util import run_dbt, run_dbt_and_capture + +from tests.functional.generic_test_tests import _files + + +class TestWarehouseConfig: + + @pytest.fixture(scope="class") + def seeds(self): + return { + "colors.csv": _files.SEED__COLORS, + "facts.csv": _files.SEED__FACTS, + } + + @pytest.fixture(scope="class", autouse=True) + def setup(self, project): + run_dbt(["seed"]) + run_dbt(["run"]) + yield + + +class TestWarehouseConfigControl(TestWarehouseConfig): + + @pytest.fixture(scope="class") + def models(self): + return {"schema.yml": _files.SCHEMA__CONTROL} + + def test_expected_warehouse(self, project): + results, logs = run_dbt_and_capture(["test"]) + assert len(results) == 1 + + +class TestWarehouseConfigExplicitWarehouse(TestWarehouseConfig): + + @pytest.fixture(scope="class") + def models(self): + return {"schema.yml": _files.SCHEMA__EXPLICIT_WAREHOUSE} + + def test_expected_warehouse(self, project): + _, logs = run_dbt_and_capture(["test", "--log-level", "debug"]) + assert "use warehouse " in logs + + +class TestWarehouseConfigNotNull(TestWarehouseConfig): + + @pytest.fixture(scope="class") + def models(self): + return {"schema.yml": _files.SCHEMA__NOT_NULL} + + def test_expected_warehouse(self, project): + _, logs = run_dbt_and_capture(["test", "--log-level", "debug"], expect_pass=False) + assert "use warehouse " in logs diff --git a/tests/unit/test_adapter_telemetry.py b/tests/unit/test_adapter_telemetry.py new file mode 100644 index 000000000..498676b77 --- /dev/null +++ b/tests/unit/test_adapter_telemetry.py @@ -0,0 +1,27 @@ +from unittest import mock + +import dbt.adapters.snowflake.__version__ + +from dbt.adapters.snowflake.impl import SnowflakeAdapter +from dbt.adapters.base.relation import AdapterTrackingRelationInfo + + +def test_telemetry_with_snowflake_details(): + mock_model_config = mock.MagicMock() + mock_model_config._extra = mock.MagicMock() + mock_model_config._extra = { + "adapter_type": "snowflake", + "table_format": "iceberg", + } + + res = SnowflakeAdapter.get_adapter_run_info(mock_model_config) + + assert res.adapter_name == "snowflake" + assert res.base_adapter_version == dbt.adapters.__about__.version + assert res.adapter_version == dbt.adapters.snowflake.__version__.version + assert res.model_adapter_details == { + "adapter_type": "snowflake", + "table_format": "iceberg", + } + + assert type(res) is AdapterTrackingRelationInfo diff --git a/tests/unit/test_snowflake_adapter.py b/tests/unit/test_snowflake_adapter.py index 32e73eb45..aa580aad2 100644 --- a/tests/unit/test_snowflake_adapter.py +++ b/tests/unit/test_snowflake_adapter.py @@ -60,8 +60,10 @@ def setUp(self): self.handle = mock.MagicMock(spec=snowflake_connector.SnowflakeConnection) self.cursor = self.handle.cursor.return_value self.mock_execute = self.cursor.execute + self.mock_execute.return_value = mock.MagicMock(sfqid="42") self.patcher = mock.patch("dbt.adapters.snowflake.connections.snowflake.connector.connect") self.snowflake = self.patcher.start() + self.snowflake.connect.cursor.return_value = mock.MagicMock(sfqid="42") # Create the Manifest.state_check patcher @mock.patch("dbt.parser.manifest.ManifestLoader.build_manifest_state_check") @@ -90,7 +92,6 @@ def _mock_state_check(self): self.qh_patch = mock.patch.object(self.adapter.connections.query_header, "add") self.mock_query_header_add = self.qh_patch.start() self.mock_query_header_add.side_effect = lambda q: "/* dbt */\n{}".format(q) - self.adapter.acquire_connection() inject_adapter(self.adapter, SnowflakePlugin)