Skip to content
This repository was archived by the owner on Apr 8, 2024. It is now read-only.

fix(dbt-fal): handle new relation quoting for Python introduced in dbt-1.4.5 #781

Merged
merged 1 commit into from
Mar 13, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ sources:
warn_after: { "count": 5, "period": minute }
error_after: { "count": 30, "period": minute }
tables:
- name: "freshness_table"
- name: freshness_table
loaded_at_field: "current_timestamp"
columns:
- name: info
Expand Down
5 changes: 2 additions & 3 deletions projects/adapter/src/dbt/adapters/fal/impl.py
Original file line number Diff line number Diff line change
Expand Up @@ -134,10 +134,9 @@ def __new__(cls, config):

# TODO: maybe we can do this better?
with _release_plugin_lock():
original_plugin = FACTORY.get_plugin_by_name(fal_credentials.type)
db_adapter_class = FACTORY.get_adapter_class_by_name(db_credentials.type)

original_plugin.dependencies = [db_credentials.type]
original_plugin = FACTORY.get_plugin_by_name(fal_credentials.type)
original_plugin.dependencies = [db_credentials.type]

config.python_adapter_credentials = fal_credentials
config.sql_adapter_credentials = db_credentials
Expand Down
2 changes: 2 additions & 0 deletions projects/adapter/src/dbt/adapters/fal/wrappers.py
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,8 @@ def manifest(self):
return ManifestLoader.get_full_manifest(self.config)

def type(self):
# NOTE: This does not let `fal__` macros to be used
# Maybe for 1.5 we will get a more reliable way to detect if we are in a SQL or Python context
if find_funcs_in_stack({"render", "db_materialization"}):
return self._db_adapter.type()

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,9 @@ def _get_alchemy_engine(adapter: BaseAdapter, connection: Connection) -> Any:

sqlalchemy_kwargs = {}
format_url = lambda url: url
if adapter_type == 'trino':
if adapter_type == "trino":
import dbt.adapters.fal_experimental.support.trino as support_trino

return support_trino.create_engine(adapter)

if adapter_type == "redshift":
Expand All @@ -40,7 +41,7 @@ def _get_alchemy_engine(adapter: BaseAdapter, connection: Connection) -> Any:
f"dbt-fal does not support {adapter_type} adapter. ",
f"If you need {adapter_type} support, you can create an issue ",
"in our GitHub repository: https://github.com/fal-ai/fal. ",
"We will look into it ASAP."
"We will look into it ASAP.",
)
raise NotImplementedError(message)

Expand Down Expand Up @@ -156,15 +157,25 @@ def prepare_for_adapter(adapter: BaseAdapter, function: Any) -> Any:

@functools.wraps(function)
def wrapped(quoted_relation: str, *args, **kwargs) -> Any:
relation = adapter.Relation.create(
*quoted_relation.split("."), type=RelationType.Table
# HACK: we need to drop the quotes from the relation parts
# This was introduced in https://github.com/dbt-labs/dbt-core/pull/7115
# and the recommended solution would be to create a macro `fal__resolve_model_name`
# but it is not possible thanks a macro resolution error we get by returning the db_adapter type.
# The overall solution could be to avoid creating a Relation and just passing the string as is to the read/write functions.
parts = map(
lambda part: part.strip(adapter.Relation.quote_character),
[*quoted_relation.split(".")],
)

relation = adapter.Relation.create(*parts, type=RelationType.Table)
return function(adapter, relation, *args, **kwargs)

return wrapped


def reconstruct_adapter(config: RuntimeConfig, manifest: Manifest, macro_manifest: MacroManifest) -> BaseAdapter:
def reconstruct_adapter(
config: RuntimeConfig, manifest: Manifest, macro_manifest: MacroManifest
) -> BaseAdapter:
from dbt.tracking import do_not_track

# Prepare the DBT to not to track us.
Expand Down