Skip to content
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.

Sign up for GitHub

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

Propose fix for connection testing issues in Cloud. #836

Merged
merged 4 commits into from
Oct 24, 2024
+14 −4
Merged

Propose fix for connection testing issues in Cloud. #836

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8054bcb
Propose fix for connection testing issues in Cloud.
VersusFacit Oct 24, 2024
0830e3a
edit changelog.
VersusFacit Oct 24, 2024
47b19ff
fix missing import
benc-db Oct 24, 2024
b4db99a
Use stricter case for test to guarantee as far as possible dbt_projec…
VersusFacit Oct 24, 2024
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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
- Fix places where we were not properly closing cursors, and other test warnings ([713](https://github.com/databricks/dbt-databricks/pull/713))
- Drop support for Python 3.8 ([713](https://github.com/databricks/dbt-databricks/pull/713))
- Upgrade databricks-sql-connector dependency to 3.5.0 ([833](https://github.com/databricks/dbt-databricks/pull/833))
- Fix behavior flag use in init of DatabricksAdapter ([836](https://github.com/databricks/dbt-databricks/pull/836/files))

## dbt-databricks 1.8.7 (October 10, 2024)

Expand Down
13 changes: 9 additions & 4 deletions dbt/adapters/databricks/impl.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,10 +182,15 @@ class DatabricksAdapter(SparkAdapter):

def __init__(self, config: Any, mp_context: SpawnContext) -> None:
super().__init__(config, mp_context)
if self.behavior.use_info_schema_for_columns.no_warn: # type: ignore[attr-defined]
self.get_column_behavior = GetColumnsByInformationSchema()
else:
self.get_column_behavior = GetColumnsByDescribe()

# dbt doesn't propogate flags for certain workflows like dbt debug so this requires
# an additional guard
self.get_column_behavior = GetColumnsByDescribe()
try:
if self.behavior.use_info_schema_for_columns.no_warn: # type: ignore[attr-defined]
self.get_column_behavior = GetColumnsByInformationSchema()
except CompilationError:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we didn't expect behavior flags to be used this early in the runtime, so we need an extra check for the use here that protects against situations where flags don't get registered

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would move this condition into get_columns_in_relation. It looks like that's the only thing that references self.get_column"_behavior (at least in the adapter itself). And if we're at that point, then we definitely have flags from project.yml. This also aligns with pushing behavior flags as close to their relevant code as possible. While column metadata is a part of the vast majority of runs, we should still strive to keep the flag close to the code that it impacts, that way when we remove the flag we have a better idea of what we're affecting.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this not defined by init time? In general I try to follow the pattern of https://blog.ploeh.dk/2011/07/28/CompositionRoot/, but man does dbt make that difficult. I would strongly prefer not to delay selecting behavior until get_columns_in_relation. I am fine with @VersusFacit's suggestion, but don't really understand why the initialization behavior is not the same for all paths.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We took this thread to a more rapid response forum but for posterity project flags come from the dbt_project.yml which cloud doesn't always have in scope for certain workflows. So for that situation, we pass a dummy dbt_project.yml (empty of flags) and use profiles.yml / the cloud equivalent to, say, connection test.

VersusFacit marked this conversation as resolved.
Show resolved Hide resolved
pass

@property
def _behavior_flags(self) -> List[BehaviorFlag]:
Expand Down
3 changes: 3 additions & 0 deletions tests/functional/adapter/columns/test_get_columns.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@ def models(self):

@pytest.fixture(scope="class", autouse=True)
def setup(self, project):
# debug uses different rules for managing project flags than run
util.run_dbt(["debug"])

util.run_dbt(["run"])

@pytest.fixture(scope="class")
Expand Down
Loading