From ca81eeab01a4d33534284c98c87e5a35a3f3668b Mon Sep 17 00:00:00 2001 From: Quigley Malcolm Date: Tue, 12 Nov 2024 15:03:04 -0600 Subject: [PATCH 1/6] Fix setting of global behavior flags --- .changes/unreleased/Fixes-20241112-143740.yaml | 6 ++++++ dbt/adapters/base/impl.py | 9 ++++++++- 2 files changed, 14 insertions(+), 1 deletion(-) create mode 100644 .changes/unreleased/Fixes-20241112-143740.yaml diff --git a/.changes/unreleased/Fixes-20241112-143740.yaml b/.changes/unreleased/Fixes-20241112-143740.yaml new file mode 100644 index 00000000..567a1e9b --- /dev/null +++ b/.changes/unreleased/Fixes-20241112-143740.yaml @@ -0,0 +1,6 @@ +kind: Fixes +body: Move require_batched_execution_for_custom_microbatch_strategy flag to gloabl +time: 2024-11-12T14:37:40.681284-06:00 +custom: + Author: QMalcolm + Issue: N/A diff --git a/dbt/adapters/base/impl.py b/dbt/adapters/base/impl.py index c8457e2f..c173ab8e 100644 --- a/dbt/adapters/base/impl.py +++ b/dbt/adapters/base/impl.py @@ -262,6 +262,13 @@ class BaseAdapter(metaclass=AdapterMeta): } MAX_SCHEMA_METADATA_RELATIONS = 100 + GLOBAL_BEHAVIOR_FLAGS = [ + { + "name": "require_batched_execution_for_custom_microbatch_strategy", + "default": False, + "docs_url": "https://docs.getdbt.com/docs/build/incremental-microbatch", + } + ] # This static member variable can be overridden in concrete adapter # implementations to indicate adapter support for optional capabilities. @@ -274,7 +281,7 @@ def __init__(self, config, mp_context: SpawnContext) -> None: self._macro_resolver: Optional[MacroResolverProtocol] = None self._macro_context_generator: Optional[MacroContextGeneratorCallable] = None # this will be updated to include global behavior flags once they exist - self.behavior = [] # type: ignore + self.behavior = self.GLOBAL_BEHAVIOR_FLAGS # type: ignore ### # Methods to set / access a macro resolver From 06683b92294f8870084303753ba43c2d9cd6d374 Mon Sep 17 00:00:00 2001 From: Michelle Ark Date: Tue, 12 Nov 2024 16:39:17 -0500 Subject: [PATCH 2/6] remove global flag from base _behavior_flags --- .changes/unreleased/Fixes-20241112-143740.yaml | 4 ++-- dbt/adapters/base/impl.py | 8 +------- 2 files changed, 3 insertions(+), 9 deletions(-) diff --git a/.changes/unreleased/Fixes-20241112-143740.yaml b/.changes/unreleased/Fixes-20241112-143740.yaml index 567a1e9b..5d5e7560 100644 --- a/.changes/unreleased/Fixes-20241112-143740.yaml +++ b/.changes/unreleased/Fixes-20241112-143740.yaml @@ -1,6 +1,6 @@ kind: Fixes -body: Move require_batched_execution_for_custom_microbatch_strategy flag to gloabl +body: Move require_batched_execution_for_custom_microbatch_strategy flag to global time: 2024-11-12T14:37:40.681284-06:00 custom: Author: QMalcolm - Issue: N/A + Issue: 327 diff --git a/dbt/adapters/base/impl.py b/dbt/adapters/base/impl.py index c173ab8e..e6d20b57 100644 --- a/dbt/adapters/base/impl.py +++ b/dbt/adapters/base/impl.py @@ -322,13 +322,7 @@ def _behavior_flags(self) -> List[BehaviorFlag]: """ This method should be overwritten by adapter maintainers to provide platform-specific flags """ - return [ - { - "name": "require_batched_execution_for_custom_microbatch_strategy", - "default": False, - "docs_url": "https://docs.getdbt.com/docs/build/incremental-microbatch", - } - ] + return [] ### # Methods that pass through to the connection manager From b3a6be7416d2bb73cf2eccf338f03752261f63c0 Mon Sep 17 00:00:00 2001 From: Michelle Ark Date: Tue, 12 Nov 2024 17:45:35 -0500 Subject: [PATCH 3/6] add test_behaviour_flags_property_empty --- dbt/adapters/base/impl.py | 2 + .../test_behavior_flags.py | 4 + tests/unit/conftest.py | 2 +- tests/unit/fixtures/__init__.py | 2 +- tests/unit/fixtures/adapter.py | 189 +++++++++--------- 5 files changed, 106 insertions(+), 93 deletions(-) diff --git a/dbt/adapters/base/impl.py b/dbt/adapters/base/impl.py index e6d20b57..f1746a7f 100644 --- a/dbt/adapters/base/impl.py +++ b/dbt/adapters/base/impl.py @@ -321,6 +321,8 @@ def behavior(self, flags: List[BehaviorFlag]) -> None: def _behavior_flags(self) -> List[BehaviorFlag]: """ This method should be overwritten by adapter maintainers to provide platform-specific flags + + The BaseAdapter should NOT include any global flags here as should be defined in BaseAdapter.GLOBAL_BEHAVIOR_FLAGS """ return [] diff --git a/tests/unit/behavior_flag_tests/test_behavior_flags.py b/tests/unit/behavior_flag_tests/test_behavior_flags.py index 7f3abb89..d272ab11 100644 --- a/tests/unit/behavior_flag_tests/test_behavior_flags.py +++ b/tests/unit/behavior_flag_tests/test_behavior_flags.py @@ -64,3 +64,7 @@ def test_register_behavior_flags(adapter): assert not adapter.behavior.default_true_user_false_flag assert adapter.behavior.default_true_user_true_flag assert adapter.behavior.default_true_user_skip_flag + + +def test_behaviour_flags_property_empty(adapter_default_behaviour_flags): + assert adapter_default_behaviour_flags._behavior_flags == [] diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py index 346634df..d837bce7 100644 --- a/tests/unit/conftest.py +++ b/tests/unit/conftest.py @@ -1 +1 @@ -from tests.unit.fixtures import adapter, behavior_flags, config, flags +from tests.unit.fixtures import adapter, adapter_default_behaviour_flags, behavior_flags, config, flags diff --git a/tests/unit/fixtures/__init__.py b/tests/unit/fixtures/__init__.py index 78135a2c..f07ef874 100644 --- a/tests/unit/fixtures/__init__.py +++ b/tests/unit/fixtures/__init__.py @@ -1 +1 @@ -from tests.unit.fixtures.adapter import adapter, behavior_flags, config, flags +from tests.unit.fixtures.adapter import adapter, adapter_default_behaviour_flags, behavior_flags, config, flags diff --git a/tests/unit/fixtures/adapter.py b/tests/unit/fixtures/adapter.py index b59b0423..215916aa 100644 --- a/tests/unit/fixtures/adapter.py +++ b/tests/unit/fixtures/adapter.py @@ -15,105 +15,112 @@ from tests.unit.fixtures.credentials import CredentialsStub -@pytest.fixture -def adapter(config, behavior_flags) -> BaseAdapter: +class BaseAdapterStub(BaseAdapter): + """ + A stub for an adapter that uses the cache as the database + """ + + ConnectionManager = ConnectionManagerStub + + ### + # Abstract methods for database-specific values, attributes, and types + ### + @classmethod + def date_function(cls) -> str: + return "date_function" + + @classmethod + def is_cancelable(cls) -> bool: + return False + + def list_schemas(self, database: str) -> List[str]: + return list(self.cache.schemas) + + ### + # Abstract methods about relations + ### + def drop_relation(self, relation: BaseRelation) -> None: + self.cache_dropped(relation) + + def truncate_relation(self, relation: BaseRelation) -> None: + self.cache_dropped(relation) + + def rename_relation(self, from_relation: BaseRelation, to_relation: BaseRelation) -> None: + self.cache_renamed(from_relation, to_relation) + + def get_columns_in_relation(self, relation: BaseRelation) -> List[Column]: + # there's no database, so these need to be added as kwargs in the existing_relations fixture + return relation.columns + + def expand_column_types(self, goal: BaseRelation, current: BaseRelation) -> None: + # there's no database, so these need to be added as kwargs in the existing_relations fixture + object.__setattr__(current, "columns", goal.columns) + + def list_relations_without_caching( + self, schema_relation: BaseRelation + ) -> List[BaseRelation]: + # there's no database, so use the cache as the database + return self.cache.get_relations(schema_relation.database, schema_relation.schema) + + ### + # ODBC FUNCTIONS -- these should not need to change for every adapter, + # although some adapters may override them + ### + def create_schema(self, relation: BaseRelation): + # there's no database, this happens implicitly by adding a relation to the cache + pass + + def drop_schema(self, relation: BaseRelation): + for each_relation in self.cache.get_relations(relation.database, relation.schema): + self.cache_dropped(each_relation) + + @classmethod + def quote(cls, identifier: str) -> str: + quote_char = "" + return f"{quote_char}{identifier}{quote_char}" + + ### + # Conversions: These must be implemented by concrete implementations, for + # converting agate types into their sql equivalents. + ### + @classmethod + def convert_text_type(cls, agate_table: agate.Table, col_idx: int) -> str: + return "str" + + @classmethod + def convert_number_type(cls, agate_table: agate.Table, col_idx: int) -> str: + return "float" + + @classmethod + def convert_boolean_type(cls, agate_table: agate.Table, col_idx: int) -> str: + return "bool" + + @classmethod + def convert_datetime_type(cls, agate_table: agate.Table, col_idx: int) -> str: + return "datetime" + + @classmethod + def convert_date_type(cls, *args, **kwargs): + return "date" + + @classmethod + def convert_time_type(cls, *args, **kwargs): + return "time" - class BaseAdapterStub(BaseAdapter): - """ - A stub for an adapter that uses the cache as the database - """ - ConnectionManager = ConnectionManagerStub +@pytest.fixture +def adapter(config, behavior_flags) -> BaseAdapter: + class BaseAdapterBehaviourFlagStub(BaseAdapterStub): @property def _behavior_flags(self) -> List[BehaviorFlag]: return behavior_flags - ### - # Abstract methods for database-specific values, attributes, and types - ### - @classmethod - def date_function(cls) -> str: - return "date_function" - - @classmethod - def is_cancelable(cls) -> bool: - return False - - def list_schemas(self, database: str) -> List[str]: - return list(self.cache.schemas) - - ### - # Abstract methods about relations - ### - def drop_relation(self, relation: BaseRelation) -> None: - self.cache_dropped(relation) - - def truncate_relation(self, relation: BaseRelation) -> None: - self.cache_dropped(relation) - - def rename_relation(self, from_relation: BaseRelation, to_relation: BaseRelation) -> None: - self.cache_renamed(from_relation, to_relation) - - def get_columns_in_relation(self, relation: BaseRelation) -> List[Column]: - # there's no database, so these need to be added as kwargs in the existing_relations fixture - return relation.columns - - def expand_column_types(self, goal: BaseRelation, current: BaseRelation) -> None: - # there's no database, so these need to be added as kwargs in the existing_relations fixture - object.__setattr__(current, "columns", goal.columns) - - def list_relations_without_caching( - self, schema_relation: BaseRelation - ) -> List[BaseRelation]: - # there's no database, so use the cache as the database - return self.cache.get_relations(schema_relation.database, schema_relation.schema) - - ### - # ODBC FUNCTIONS -- these should not need to change for every adapter, - # although some adapters may override them - ### - def create_schema(self, relation: BaseRelation): - # there's no database, this happens implicitly by adding a relation to the cache - pass - - def drop_schema(self, relation: BaseRelation): - for each_relation in self.cache.get_relations(relation.database, relation.schema): - self.cache_dropped(each_relation) - - @classmethod - def quote(cls, identifier: str) -> str: - quote_char = "" - return f"{quote_char}{identifier}{quote_char}" - - ### - # Conversions: These must be implemented by concrete implementations, for - # converting agate types into their sql equivalents. - ### - @classmethod - def convert_text_type(cls, agate_table: agate.Table, col_idx: int) -> str: - return "str" - - @classmethod - def convert_number_type(cls, agate_table: agate.Table, col_idx: int) -> str: - return "float" - - @classmethod - def convert_boolean_type(cls, agate_table: agate.Table, col_idx: int) -> str: - return "bool" - - @classmethod - def convert_datetime_type(cls, agate_table: agate.Table, col_idx: int) -> str: - return "datetime" - - @classmethod - def convert_date_type(cls, *args, **kwargs): - return "date" - - @classmethod - def convert_time_type(cls, *args, **kwargs): - return "time" + return BaseAdapterBehaviourFlagStub(config, get_context("spawn")) + +@pytest.fixture +def adapter_default_behaviour_flags(config) -> BaseAdapter: return BaseAdapterStub(config, get_context("spawn")) From 39a7d40301bdbf0bf0e960a38e5d4c33bc875a5c Mon Sep 17 00:00:00 2001 From: Michelle Ark Date: Tue, 12 Nov 2024 17:52:26 -0500 Subject: [PATCH 4/6] linting --- tests/unit/conftest.py | 8 +++++++- tests/unit/fixtures/__init__.py | 8 +++++++- tests/unit/fixtures/adapter.py | 4 +--- 3 files changed, 15 insertions(+), 5 deletions(-) diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py index d837bce7..225bdf57 100644 --- a/tests/unit/conftest.py +++ b/tests/unit/conftest.py @@ -1 +1,7 @@ -from tests.unit.fixtures import adapter, adapter_default_behaviour_flags, behavior_flags, config, flags +from tests.unit.fixtures import ( + adapter, + adapter_default_behaviour_flags, + behavior_flags, + config, + flags, +) diff --git a/tests/unit/fixtures/__init__.py b/tests/unit/fixtures/__init__.py index f07ef874..caa1448f 100644 --- a/tests/unit/fixtures/__init__.py +++ b/tests/unit/fixtures/__init__.py @@ -1 +1,7 @@ -from tests.unit.fixtures.adapter import adapter, adapter_default_behaviour_flags, behavior_flags, config, flags +from tests.unit.fixtures.adapter import ( + adapter, + adapter_default_behaviour_flags, + behavior_flags, + config, + flags, +) diff --git a/tests/unit/fixtures/adapter.py b/tests/unit/fixtures/adapter.py index 215916aa..3730a083 100644 --- a/tests/unit/fixtures/adapter.py +++ b/tests/unit/fixtures/adapter.py @@ -56,9 +56,7 @@ def expand_column_types(self, goal: BaseRelation, current: BaseRelation) -> None # there's no database, so these need to be added as kwargs in the existing_relations fixture object.__setattr__(current, "columns", goal.columns) - def list_relations_without_caching( - self, schema_relation: BaseRelation - ) -> List[BaseRelation]: + def list_relations_without_caching(self, schema_relation: BaseRelation) -> List[BaseRelation]: # there's no database, so use the cache as the database return self.cache.get_relations(schema_relation.database, schema_relation.schema) From b0441a0d637e7efdba9d7ac5133b5b79b8b2b6af Mon Sep 17 00:00:00 2001 From: Quigley Malcolm Date: Tue, 12 Nov 2024 18:56:05 -0600 Subject: [PATCH 5/6] Move list of default behavior flags outside of `BaseAdapter` class --- dbt/adapters/base/impl.py | 19 +++++++++---------- .../test_behavior_flags.py | 6 ++++++ 2 files changed, 15 insertions(+), 10 deletions(-) diff --git a/dbt/adapters/base/impl.py b/dbt/adapters/base/impl.py index f1746a7f..15093600 100644 --- a/dbt/adapters/base/impl.py +++ b/dbt/adapters/base/impl.py @@ -98,6 +98,13 @@ GET_CATALOG_RELATIONS_MACRO_NAME = "get_catalog_relations" FRESHNESS_MACRO_NAME = "collect_freshness" GET_RELATION_LAST_MODIFIED_MACRO_NAME = "get_relation_last_modified" +DEFAULT_BASE_BEHAVIOR_FLAGS = [ + { + "name": "require_batched_execution_for_custom_microbatch_strategy", + "default": False, + "docs_url": "https://docs.getdbt.com/docs/build/incremental-microbatch", + } +] class ConstraintSupport(str, Enum): @@ -262,13 +269,6 @@ class BaseAdapter(metaclass=AdapterMeta): } MAX_SCHEMA_METADATA_RELATIONS = 100 - GLOBAL_BEHAVIOR_FLAGS = [ - { - "name": "require_batched_execution_for_custom_microbatch_strategy", - "default": False, - "docs_url": "https://docs.getdbt.com/docs/build/incremental-microbatch", - } - ] # This static member variable can be overridden in concrete adapter # implementations to indicate adapter support for optional capabilities. @@ -280,8 +280,7 @@ def __init__(self, config, mp_context: SpawnContext) -> None: self.connections = self.ConnectionManager(config, mp_context) self._macro_resolver: Optional[MacroResolverProtocol] = None self._macro_context_generator: Optional[MacroContextGeneratorCallable] = None - # this will be updated to include global behavior flags once they exist - self.behavior = self.GLOBAL_BEHAVIOR_FLAGS # type: ignore + self.behavior = DEFAULT_BASE_BEHAVIOR_FLAGS # type: ignore ### # Methods to set / access a macro resolver @@ -322,7 +321,7 @@ def _behavior_flags(self) -> List[BehaviorFlag]: """ This method should be overwritten by adapter maintainers to provide platform-specific flags - The BaseAdapter should NOT include any global flags here as should be defined in BaseAdapter.GLOBAL_BEHAVIOR_FLAGS + The BaseAdapter should NOT include any global flags here as those should be defined via DEFAULT_BASE_BEHAVIOR_FLAGS """ return [] diff --git a/tests/unit/behavior_flag_tests/test_behavior_flags.py b/tests/unit/behavior_flag_tests/test_behavior_flags.py index d272ab11..378d07bb 100644 --- a/tests/unit/behavior_flag_tests/test_behavior_flags.py +++ b/tests/unit/behavior_flag_tests/test_behavior_flags.py @@ -1,5 +1,6 @@ from typing import Any, Dict, List +from dbt.adapters.base.impl import DEFAULT_BASE_BEHAVIOR_FLAGS from dbt_common.behavior_flags import BehaviorFlag from dbt_common.exceptions import DbtBaseException import pytest @@ -68,3 +69,8 @@ def test_register_behavior_flags(adapter): def test_behaviour_flags_property_empty(adapter_default_behaviour_flags): assert adapter_default_behaviour_flags._behavior_flags == [] + + +def test_behavior_property_has_defaults(adapter_default_behaviour_flags): + for flag in DEFAULT_BASE_BEHAVIOR_FLAGS: + assert hasattr(adapter_default_behaviour_flags.behavior, flag["name"]) From 9671239a435b85a572d165d06c314d25dca81f87 Mon Sep 17 00:00:00 2001 From: Quigley Malcolm Date: Tue, 12 Nov 2024 20:18:51 -0600 Subject: [PATCH 6/6] Give changelog a better issue number --- .changes/unreleased/Fixes-20241112-143740.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.changes/unreleased/Fixes-20241112-143740.yaml b/.changes/unreleased/Fixes-20241112-143740.yaml index 5d5e7560..ca899cbc 100644 --- a/.changes/unreleased/Fixes-20241112-143740.yaml +++ b/.changes/unreleased/Fixes-20241112-143740.yaml @@ -2,5 +2,5 @@ kind: Fixes body: Move require_batched_execution_for_custom_microbatch_strategy flag to global time: 2024-11-12T14:37:40.681284-06:00 custom: - Author: QMalcolm - Issue: 327 + Author: QMalcolm MichelleArk + Issue: 351