ADAP-845: Update BaseAdapter to meet mypy standards #8485

mikealfare · 2023-08-23T23:46:31Z

resolves #8396

Problem

mypy throws a lot of warnings that should be addressed.

Solution

Address the warnings by adding/updating types, checking types in code, or moving abstract methods towards the base class.

Checklist

I have read the contributing guide and understand what's expected of me
I have run this code in development and it appears to resolve the stated issue
This PR includes tests, or tests are not required/relevant for this PR
This PR has no interface changes (e.g. macros, cli, logs, json artifacts, config files, adapter interface, etc) or this PR has already received feedback and approval from Product or DX

codecov · 2023-08-23T23:49:15Z

Codecov Report

Merging #8485 (a5a02b4) into main (4d3c6d9) will decrease coverage by 0.02%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##             main    #8485      +/-   ##
==========================================
- Coverage   86.36%   86.34%   -0.02%     
==========================================
  Files         174      174              
  Lines       25568    25573       +5     
==========================================
+ Hits        22081    22082       +1     
- Misses       3487     3491       +4

Flag	Coverage Δ
integration	`83.20% <66.66%> (-0.03%)`	⬇️
unit	`65.13% <46.66%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
core/dbt/adapters/base/connections.py	`89.24% <60.00%> (-0.81%)`	⬇️
core/dbt/adapters/base/impl.py	`83.57% <66.66%> (-0.25%)`	⬇️
core/dbt/adapters/sql/connections.py	`95.91% <100.00%> (+0.86%)`	⬆️

... and 1 file with indirect coverage changes

mikealfare · 2023-08-23T23:50:23Z

core/dbt/adapters/sql/connections.py

@@ -131,14 +131,6 @@ def get_result_from_cursor(cls, cursor: Any, limit: Optional[int]) -> agate.Tabl

        return dbt.clients.agate_helper.table_from_data_flat(data, column_names)

-    @classmethod
-    def data_type_code_to_name(cls, type_code: Union[int, str]) -> str:


This is already an abstract method. Move it up to ConnectionManager since BaseAdapter expects it to exist for some methods.

mikealfare · 2023-08-23T23:51:35Z

core/dbt/adapters/base/impl.py

@@ -315,14 +315,21 @@ def get_column_schema_from_query(self, sql: str) -> List[BaseColumn]:

    @available.parse(lambda *a, **k: ("", empty_table()))
    def get_partitions_metadata(self, table: str) -> Tuple[agate.Table]:


This should probably live in dbt-bigquery, based on the comment. But to preserve backwards compatibility, I checked that the method exists prior to calling it. This makes mypy happy while preserving backwards compatibility.

mikealfare · 2023-08-23T23:53:11Z

core/dbt/adapters/base/impl.py

        for relation in cache_schemas:
-            cache_update.add((relation.database, relation.schema))
+            if relation.schema:


mypy balked at relation.schema potentially not existing. That's because BaseRelation lists all of its parts as optional (though at least one must exist). In this scenario, we would expect schema to exist, as the type above suggests. Adding this check satisfies mypy and doesn't deviate much from the existing code.

mikealfare · 2023-08-23T23:53:48Z

core/dbt/adapters/base/impl.py

@@ -453,9 +460,10 @@ def _relations_cache_for_schemas(
        # it's possible that there were no relations in some schemas. We want
        # to insert the schemas we query into the cache's `.schemas` attribute
        # so we can check it later
-        cache_update: Set[Tuple[Optional[str], Optional[str]]] = set()
+        cache_update: Set[Tuple[Optional[str], str]] = set()


This type aligns with the change below and with what update_schemas expects (which is the only reason for this variable).

mikealfare · 2023-08-23T23:54:59Z

core/dbt/adapters/base/connections.py

+        )
+
+    @classmethod
+    def data_type_code_to_name(cls, type_code: Union[int, str]) -> str:


Moved up from SQLConnection.

mikealfare · 2023-08-23T23:56:27Z

core/dbt/adapters/base/connections.py

        :return: A tuple of the query status and results (empty if fetch=False).
        :rtype: Tuple[AdapterResponse, agate.Table]
        """
        raise dbt.exceptions.NotImplementedError("`execute` is not implemented for this adapter!")
+
+    def add_select_query(self, sql: str) -> Tuple[Connection, Any]:


The comment says it all. This is overridden in SQLConnectionManager, but is used in BaseAdapter, which uses ConnectionManager. This is not expected to exist today, and the BaseAdapter method only works if this method is defined anyway.

mikealfare · 2023-08-23T23:57:39Z

core/dbt/adapters/base/connections.py

@@ -400,15 +400,36 @@ def _add_query_comment(self, sql: str) -> str:

    @abc.abstractmethod
    def execute(
-        self, sql: str, auto_begin: bool = False, fetch: bool = False
+        self, sql: str, auto_begin: bool = False, fetch: bool = False, limit: Optional[int] = None


This was leftover from the implementation of limit in a previous change. The type here matches that of the original change.

gshank

Looks good! Amazing how many changes were required just by adding a return type...

mikealfare · 2023-08-24T20:21:09Z

Looks good! Amazing how many changes were required just by adding a return type...

More than half of them were the protocol that you identified. I replaced it with the actual class (ConnectionManager). We can fix the protocol later since the only thing it specified was TYPE, which as you described it, was not very useful. I didn't need to import any new modules, just a new class from an existing module, so no concern for circular references.

* Add compiled node properties to run_results.json * Include compiled-node attributes in run_results.json * Fix typo * Bump schema version of run_results * Fix test assertions * Update expected run_results to reflect new attributes * Code review changes * Fix mypy warnings for ManifestLoader.load() (#8443) * revert python version for docker images (#8445) * revert python version for docker images * add comment to not update python version, update changelog * Bumping version to 1.7.0b1 and generate changelog * [CT-3013] Fix parsing of `window_groupings` (#8454) * Update semantic model parsing tests to check measure non_additive_dimension spec * Make `window_groupings` default to empty list if not specified on `non_additive_dimension` * Add changie doc for `window_groupings` parsing fix * update `Number` class to handle integer values (#8306) * add show test for json data * oh changie my changie * revert unecessary cahnge to fixture * keep decimal class for precision methods, but return __int__ value * jerco updates * update integer type * update other tests * Update .changes/unreleased/Fixes-20230803-093502.yaml --------- Co-authored-by: Emily Rockman <emily.rockman@dbtlabs.com> * Improve docker image README (#8212) * Improve docker image README - Fix unnecessary/missing newline escapes - Remove double whitespace between parameters - 2-space indent for extra lines in image build commands * Add changelog entry for #8212 * ADAP-814: Refactor prep for MV updates (#8459) * apply reformatting changes only for #8449 * add logging back to get_create_materialized_view_as_sql * changie * swap trigger (#8463) * update the implementation template (#8466) * update the implementation template * add colon * Split tests into classes (#8474) * add flaky decorator * split up tests into classes * revert update agate for int (#8478) * updated typing and methods to meet mypy standards (#8485) * Convert error to conditional warning for unversioned contracted model, fix msg format (#8451) * first pass, tests need updates * update proto defn * fixing tests * more test fixes * finish fixing test file * reformat the message * formatting messages * changelog * add event to unit test * feedback on message structure * WIP * fix up event to take in all fields * fix test * Fix ambiguous reference error for duplicate model names across packages with tests (#8488) * Safely remove external nodes from manifest (#8495) * [CT-2840] Improved semantic layer protocol satisfaction tests (#8456) * Test `SemanticModel` satisfies protocol when none of it's `Optionals` are specified * Add tests ensuring SourceFileMetadata and FileSlice satisfiy DSI protocols * Add test asserting Defaults obj satisfies protocol * Add test asserting SemanticModel with optionals specified satisfies protocol * Split dimension protocol satisfaction tests into with and without optionals * Simplify DSI Protocol import strategy in protocol satisfaction tests * Add test asserting DimensionValidtyParams satisfies protocol * Add test asserting DimensionTypeParams satisfies protocol * Split entity protocol satisfaction tests into with and without optionals * Split measure protocol satisfication tests and add measure aggregation params satisficaition test * Split metric protocol satisfaction test into optional specified an unspecified Additionally, create where_filter pytest fixture * Improve protocol satisfaction tests for MetricTypeParams and sub protocols Specifically we added/improved protocol satisfaction tests for - MetricTypeParams - MetricInput - MetricInputMeasure - MetricTimeWindow * Convert to using mashumaro jsonschema with acceptable performance (#8437) * Regenerate run_results schema after merging in changes from main. --------- Co-authored-by: Gerda Shank <gerda@dbtlabs.com> Co-authored-by: Matthew McKnight <91097623+McKnight-42@users.noreply.github.com> Co-authored-by: Github Build Bot <buildbot@fishtownanalytics.com> Co-authored-by: Quigley Malcolm <QMalcolm@users.noreply.github.com> Co-authored-by: dave-connors-3 <73915542+dave-connors-3@users.noreply.github.com> Co-authored-by: Emily Rockman <emily.rockman@dbtlabs.com> Co-authored-by: Jaime Martínez Rincón <jaime@jamezrin.name> Co-authored-by: Mike Alfare <13974384+mikealfare@users.noreply.github.com> Co-authored-by: Michelle Ark <MichelleArk@users.noreply.github.com>

updated typing and methods to meet mypy standards

a5a02b4

mikealfare self-assigned this Aug 23, 2023

mikealfare requested review from a team as code owners August 23, 2023 23:46

mikealfare requested review from colin-rogers-dbt and peterallenwebb August 23, 2023 23:46

cla-bot bot added the cla:yes label Aug 23, 2023

mikealfare requested a review from gshank August 23, 2023 23:46

mikealfare commented Aug 23, 2023

View reviewed changes

mikealfare mentioned this pull request Aug 23, 2023

[CT-2980] Fix untyped functions in adapters/base/ (mypy warning) #8396

Closed

mikealfare changed the title ~~Update BaseAdapter to meet mypy standards~~ ADAP-845: Update BaseAdapter to meet mypy standards Aug 24, 2023

gshank approved these changes Aug 24, 2023

View reviewed changes

mikealfare merged commit d18a74d into main Aug 24, 2023
53 of 55 checks passed

mikealfare deleted the workflow/mypy/adapters-base-impl branch August 24, 2023 20:25

peterallenwebb pushed a commit that referenced this pull request Aug 30, 2023

updated typing and methods to meet mypy standards (#8485)

a890087

tlento mentioned this pull request Nov 6, 2023

Update typing-extensions version to >=4.4 #9012

Merged

5 tasks

tlento mentioned this pull request Feb 26, 2024

Update dbt-semantic-interfaces dependency to compatible range #9671

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ADAP-845: Update BaseAdapter to meet mypy standards #8485

ADAP-845: Update BaseAdapter to meet mypy standards #8485

mikealfare commented Aug 23, 2023

codecov bot commented Aug 23, 2023 •

edited

Loading

mikealfare Aug 23, 2023

mikealfare Aug 23, 2023

mikealfare Aug 23, 2023

mikealfare Aug 23, 2023

mikealfare Aug 23, 2023

mikealfare Aug 23, 2023

mikealfare Aug 23, 2023

gshank left a comment

mikealfare commented Aug 24, 2023

		@@ -315,14 +315,21 @@ def get_column_schema_from_query(self, sql: str) -> List[BaseColumn]:

		@available.parse(lambda a, *k: ("", empty_table()))
		def get_partitions_metadata(self, table: str) -> Tuple[agate.Table]:

ADAP-845: Update BaseAdapter to meet mypy standards #8485

ADAP-845: Update BaseAdapter to meet mypy standards #8485

Conversation

mikealfare commented Aug 23, 2023

Problem

Solution

Checklist

codecov bot commented Aug 23, 2023 • edited Loading

Codecov Report

mikealfare Aug 23, 2023

Choose a reason for hiding this comment

mikealfare Aug 23, 2023

Choose a reason for hiding this comment

mikealfare Aug 23, 2023

Choose a reason for hiding this comment

mikealfare Aug 23, 2023

Choose a reason for hiding this comment

mikealfare Aug 23, 2023

Choose a reason for hiding this comment

mikealfare Aug 23, 2023

Choose a reason for hiding this comment

mikealfare Aug 23, 2023

Choose a reason for hiding this comment

gshank left a comment

Choose a reason for hiding this comment

mikealfare commented Aug 24, 2023

codecov bot commented Aug 23, 2023 •

edited

Loading