-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
remove manifest from adapter.get_catalog signature #9212
Changes from all commits
7f777f8
2cee865
160d0db
d7d5e23
bc49504
b52d6f8
b8de881
ba53f05
e6b88dd
704863e
0745c89
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
kind: Under the Hood | ||
body: Remove usage of dbt.contracts in dbt/adapters | ||
time: 2023-12-05T12:05:59.936775+09:00 | ||
custom: | ||
Author: michelleark | ||
Issue: "9208" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
kind: Under the Hood | ||
body: Introduce RelationConfig Protocol, consolidate Relation.create_from | ||
time: 2023-12-05T17:07:25.33861+09:00 | ||
custom: | ||
Author: michelleark | ||
Issue: "9215" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
kind: Under the Hood | ||
body: ' remove manifest from adapter catalog method signatures' | ||
time: 2023-12-06T00:03:43.824252+09:00 | ||
custom: | ||
Author: michelleark | ||
Issue: "9218" |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,7 +4,6 @@ | |
from datetime import datetime | ||
from enum import Enum | ||
import time | ||
from itertools import chain | ||
from typing import ( | ||
Any, | ||
Callable, | ||
|
@@ -19,6 +18,7 @@ | |
Type, | ||
TypedDict, | ||
Union, | ||
FrozenSet, | ||
) | ||
from multiprocessing.context import SpawnContext | ||
|
||
|
@@ -76,6 +76,7 @@ | |
) | ||
from dbt.common.utils import filter_null_values, executor, cast_to_str, AttrDict | ||
|
||
from dbt.adapters.contracts.relation import RelationConfig | ||
from dbt.adapters.base.connections import Connection, AdapterResponse, BaseConnectionManager | ||
from dbt.adapters.base.meta import AdapterMeta, available | ||
from dbt.adapters.base.relation import ( | ||
|
@@ -110,11 +111,13 @@ | |
return row[key] | ||
|
||
|
||
def _catalog_filter_schemas(manifest: Manifest) -> Callable[[agate.Row], bool]: | ||
def _catalog_filter_schemas( | ||
used_schemas: FrozenSet[Tuple[str, str]] | ||
) -> Callable[[agate.Row], bool]: | ||
"""Return a function that takes a row and decides if the row should be | ||
included in the catalog output. | ||
""" | ||
schemas = frozenset((d.lower(), s.lower()) for d, s in manifest.get_used_schemas()) | ||
schemas = frozenset((d.lower(), s.lower()) for d, s in used_schemas) | ||
|
||
def test(row: agate.Row) -> bool: | ||
table_database = _expect_row_value("table_database", row) | ||
|
@@ -429,12 +432,12 @@ | |
""" | ||
# the cache only cares about executable nodes | ||
return { | ||
self.Relation.create_from(self.config, node).without_identifier() | ||
self.Relation.create_from(self.config, node).without_identifier() # type: ignore[arg-type] | ||
for node in manifest.nodes.values() | ||
if (node.is_relational and not node.is_ephemeral_model and not node.is_external_node) | ||
} | ||
|
||
def _get_catalog_schemas(self, manifest: Manifest) -> SchemaSearchMap: | ||
def _get_catalog_schemas(self, relation_configs: Iterable[RelationConfig]) -> SchemaSearchMap: | ||
"""Get a mapping of each node's "information_schema" relations to a | ||
set of all schemas expected in that information_schema. | ||
|
||
|
@@ -444,7 +447,7 @@ | |
lowercase strings. | ||
""" | ||
info_schema_name_map = SchemaSearchMap() | ||
relations = self._get_catalog_relations(manifest) | ||
relations = self._get_catalog_relations(relation_configs) | ||
for relation in relations: | ||
info_schema_name_map.add(relation) | ||
# result is a map whose keys are information_schema Relations without | ||
|
@@ -465,18 +468,13 @@ | |
|
||
return relations_by_info_schema | ||
|
||
def _get_catalog_relations(self, manifest: Manifest) -> List[BaseRelation]: | ||
|
||
nodes = chain( | ||
[ | ||
node | ||
for node in manifest.nodes.values() | ||
if (node.is_relational and not node.is_ephemeral_model) | ||
], | ||
manifest.sources.values(), | ||
) | ||
|
||
relations = [self.Relation.create_from(self.config, n) for n in nodes] | ||
def _get_catalog_relations( | ||
self, relation_configs: Iterable[RelationConfig] | ||
) -> List[BaseRelation]: | ||
relations = [ | ||
self.Relation.create_from(quoting=self.config, config=relation_config) | ||
for relation_config in relation_configs | ||
] | ||
return relations | ||
|
||
def _relations_cache_for_schemas( | ||
|
@@ -1122,7 +1120,9 @@ | |
return result | ||
|
||
@classmethod | ||
def _catalog_filter_table(cls, table: agate.Table, manifest: Manifest) -> agate.Table: | ||
def _catalog_filter_table( | ||
cls, table: agate.Table, used_schemas: FrozenSet[Tuple[str, str]] | ||
) -> agate.Table: | ||
"""Filter the table as appropriate for catalog entries. Subclasses can | ||
override this to change filtering rules on a per-adapter basis. | ||
""" | ||
|
@@ -1132,31 +1132,28 @@ | |
table.column_names, | ||
text_only_columns=["table_database", "table_schema", "table_name"], | ||
) | ||
return table.where(_catalog_filter_schemas(manifest)) | ||
return table.where(_catalog_filter_schemas(used_schemas)) | ||
|
||
def _get_one_catalog( | ||
self, | ||
information_schema: InformationSchema, | ||
schemas: Set[str], | ||
manifest: Manifest, | ||
used_schemas: FrozenSet[Tuple[str, str]], | ||
) -> agate.Table: | ||
kwargs = {"information_schema": information_schema, "schemas": schemas} | ||
table = self.execute_macro( | ||
GET_CATALOG_MACRO_NAME, | ||
kwargs=kwargs, | ||
# pass in the full manifest so we get any local project | ||
# overrides | ||
manifest=manifest, | ||
) | ||
|
||
results = self._catalog_filter_table(table, manifest) # type: ignore[arg-type] | ||
results = self._catalog_filter_table(table, used_schemas) # type: ignore[arg-type] | ||
return results | ||
|
||
def _get_one_catalog_by_relations( | ||
self, | ||
information_schema: InformationSchema, | ||
relations: List[BaseRelation], | ||
manifest: Manifest, | ||
used_schemas: FrozenSet[Tuple[str, str]], | ||
) -> agate.Table: | ||
|
||
kwargs = { | ||
|
@@ -1166,16 +1163,16 @@ | |
table = self.execute_macro( | ||
GET_CATALOG_RELATIONS_MACRO_NAME, | ||
kwargs=kwargs, | ||
# pass in the full manifest, so we get any local project | ||
# overrides | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This code is quite old, and I believe there is testing in place for this (will dig it up shortly) and that it is safe to remove at this point. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Found where this was introduced (4 years ago): 62755fe#diff-aa1e9e602a0f1e2930b465b30a35a28778fbe131da4fbf2c4e88dd52ca44f140R1028 and corresponding test: 62755fe#diff-0e805e1b4d683c0d06b0b1110513b7bfb4cd2f49a16a089c8de934322eca36f3R3453 |
||
manifest=manifest, | ||
) | ||
|
||
results = self._catalog_filter_table(table, manifest) # type: ignore[arg-type] | ||
results = self._catalog_filter_table(table, used_schemas) # type: ignore[arg-type] | ||
return results | ||
|
||
def get_filtered_catalog( | ||
self, manifest: Manifest, relations: Optional[Set[BaseRelation]] = None | ||
self, | ||
relation_configs: Iterable[RelationConfig], | ||
used_schemas: FrozenSet[Tuple[str, str]], | ||
relations: Optional[Set[BaseRelation]] = None, | ||
): | ||
catalogs: agate.Table | ||
if ( | ||
|
@@ -1184,11 +1181,11 @@ | |
or not self.supports(Capability.SchemaMetadataByRelations) | ||
): | ||
# Do it the traditional way. We get the full catalog. | ||
catalogs, exceptions = self.get_catalog(manifest) | ||
catalogs, exceptions = self.get_catalog(relation_configs, used_schemas) | ||
else: | ||
# Do it the new way. We try to save time by selecting information | ||
# only for the exact set of relations we are interested in. | ||
catalogs, exceptions = self.get_catalog_by_relations(manifest, relations) | ||
catalogs, exceptions = self.get_catalog_by_relations(used_schemas, relations) | ||
|
||
if relations and catalogs: | ||
relation_map = { | ||
|
@@ -1216,24 +1213,28 @@ | |
def row_matches_relation(self, row: agate.Row, relations: Set[BaseRelation]): | ||
pass | ||
|
||
def get_catalog(self, manifest: Manifest) -> Tuple[agate.Table, List[Exception]]: | ||
def get_catalog( | ||
self, | ||
relation_configs: Iterable[RelationConfig], | ||
used_schemas: FrozenSet[Tuple[str, str]], | ||
) -> Tuple[agate.Table, List[Exception]]: | ||
with executor(self.config) as tpe: | ||
futures: List[Future[agate.Table]] = [] | ||
schema_map: SchemaSearchMap = self._get_catalog_schemas(manifest) | ||
schema_map: SchemaSearchMap = self._get_catalog_schemas(relation_configs) | ||
for info, schemas in schema_map.items(): | ||
if len(schemas) == 0: | ||
continue | ||
name = ".".join([str(info.database), "information_schema"]) | ||
fut = tpe.submit_connected( | ||
self, name, self._get_one_catalog, info, schemas, manifest | ||
self, name, self._get_one_catalog, info, schemas, used_schemas | ||
) | ||
futures.append(fut) | ||
|
||
catalogs, exceptions = catch_as_completed(futures) | ||
return catalogs, exceptions | ||
|
||
def get_catalog_by_relations( | ||
self, manifest: Manifest, relations: Set[BaseRelation] | ||
self, used_schemas: FrozenSet[Tuple[str, str]], relations: Set[BaseRelation] | ||
) -> Tuple[agate.Table, List[Exception]]: | ||
with executor(self.config) as tpe: | ||
futures: List[Future[agate.Table]] = [] | ||
|
@@ -1247,7 +1248,7 @@ | |
self._get_one_catalog_by_relations, | ||
info_schema, | ||
relations, | ||
manifest, | ||
used_schemas, | ||
) | ||
futures.append(fut) | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto: https://github.com/dbt-labs/dbt-core/pull/9212/files#r1415787511