Migrate to dbt-common + dbt-adapters #342

jtcohen6 · 2024-02-19T14:54:24Z

resolves #341

Starting a draft PR for discussion!

This mostly just works by updating import statements. Caveats:

In dbt.adapters.duckdb.utils: SourceConfig and TargetConfig use type definitions that are not available from the common/adapter interfaces. We could either stub those types within duckdb.utils, or discuss if they need to be pulled into the shared interfaces.
Some of the plugin tests are failing for me locally. I'd like to see if these are real issues, or just problems with my local setup.

jwills

what a week it has been

jwills · 2024-02-23T23:12:40Z

dbt/adapters/duckdb/utils.py

@@ -75,7 +76,7 @@ def as_dict(self) -> Dict[str, Any]:
 class TargetConfig:
    relation: BaseRelation
    column_list: Sequence[Column]
-    config: RuntimeConfigObject
+    config: Any  # TODO


This is probably the safest since our usage of this config object is pretty much limited to get methods

jwills · 2024-02-23T23:13:39Z

dbt/adapters/duckdb/utils.py

@@ -47,7 +48,7 @@ def as_dict(self) -> Dict[str, Any]:
        return base

    @classmethod
-    def create_from_source(cls, source: SourceDefinition) -> "SourceConfig":
+    def create_from_source(cls, source: Any) -> "SourceConfig":


Losing the SourceDefinition typing makes me a bit more nervous since we rely on its fields pretty extensively and the external_location features are a pretty popular part of dbt-duckdb on top of simple data lakes or just CSV/Parquet files on a NAS

Heard - let me bug @MichelleArk & @colin-rogers-dbt about this and get back to you

As part of the decoupling work, we removed the BaseRelation.create_from_source and BaseRelation.create_from_node methods in favor of a consolidated BaseRelation.create_from method that accepts quoting: HasQuoting and relation_config: RelationConfig (dbt-labs/dbt-core#9210). I've updated this branch to reflect that typing + implemented DuckDBRelation.create_from here: 5712711.

We should also extend the RelationConfig to also provide meta and tags, as well as resource_type so this kind of conditional logic can still be implemented reliably for sources in dbt-adapters: https://github.com/dbt-labs/dbt-adapters/pull/120/files.

Still need to figure out how we want to handle source_meta, since its not a common property across sources + other node types. I see at a few options:

Do nothing in dbt-adapters/core, and update the duckdb create_from_source method to use hasattr or something similarly unsatisfying

Implement a merged_meta attribute on sources + nodes in dbt-core and extend RelationConfig to expect merged_meta, which the duckdb adapter could use instead of accessing source_meta + merging. This doesn't feel like the slickest interface design and would need changes to most of our node classes in core but it'd get the job done.

Potentially -- In dbt-core, update the meta field on SourceDefinition to include fields from source_meta by default, meaning the duckdb adapter wouldn't need to do the update business at all. It might be tricky to do this in a backward-compatible way in dbt-core though.

@MichelleArk thank you for this, I really appreciate it. Will try to noodle on a good strategy here, but using the hasattr stuff as an ugly-but-necessary fallback is okay with me if we don't find something better.

I went ahead with option 3 above in dbt-core: dbt-labs/dbt-core#9766, since it was actually pretty straightforward to do in a backward-compatible way (meta merges table.meta and source.meta, with table.meta taking precedence), and updated the branch to remove the source_meta reference in favor of just meta which now includes both table + source meta attributes.

I also updated the reference to config._extra to config.extra, as those should be identical and extra is already part of the RelationConfig protocol: https://github.com/dbt-labs/dbt-adapters/blob/35bd3629c390cf87a0e52d999679cc5e33f36c8f/dbt/adapters/contracts/relation.py#L41.

Tests were passing for me locally against dbt-core@main. @jwills -- could we kick off CI again?

Looks great @MichelleArk , thank you!

…xternal_sources

jwills · 2024-03-18T16:19:03Z

@MichelleArk we don't need to worry about the MD one (it requires my secret token); the code quality stuff can be fixed by running pre-commit run --all-files, I'll look at the Buena Vista failure

jtcohen6

@jwills I think this is just about ready!

Could you help me look into the Buena Vista failures? I see what appear to be connection errors:

no results to fetch
server closed the connection unexpectedly

jtcohen6 · 2024-04-16T10:05:02Z

setup.py

+        # add dbt-core to ensure backwards compatibility of installation, this is not a functional dependency
+        "dbt-core>=1.8.0b1",


Even though dbt-core is no longer a functional dependency of adapters, we're going to keep including dbt-core in setup.py to prevent surprising/breaking changes to the behavior of pip install dbt-duckdb on the day of the v1.8 final release.

dbt-duck shouldn't import anything from dbt-core directly, only from the interfaces defined in dbt-core + dbt-adapters.

jwills · 2024-04-16T14:57:23Z

@jtcohen6 thank you so much! Will take a look at the BV stuff which I think is broken on main as well; when does all of this need to be ready to go out the door?

jtcohen6 · 2024-04-16T14:59:26Z

@jwills Anytime in the next few weeks is good! Folks are beta-testing v1.8 now, we're planning to put out the release candidate (dbt-core==1.8.0rc1) on May 2.

jwills · 2024-04-16T16:41:40Z

just punting on this for now #379

…ommon-adapters-interfaces

jtcohen6 · 2024-04-19T09:55:24Z

tests/functional/adapter/test_unit_testing.py

+from dbt.tests.adapter.unit_testing.test_invalid_input import BaseUnitTestInvalidInput
+
+
+class TestUnitTestingTypesDuckDB(BaseUnitTestingTypes):


@jwills I pulled these in (selfishly), since it seemed like unit testing might "just work" on dbt-duckdb with the v1.8 interface updates.

It's all well & good on the Real Duck, but it looks like the test is failing on BuenaVista for the last data type in the list (ARRAY[1,2,3]):

File "/home/runner/work/dbt-duckdb/dbt-duckdb/.tox/buenavista/lib/python3.9/site-packages/buenavista/postgres.py", line 104, in <lambda> BVType.INTEGERARRAY: (1007, lambda v: "{" + ",".join(v) + "}"), TypeError: sequence item 0: expected str instance, int found sequence item 0: expected str instance, int found ---------------------- CSV report: buenavista_results.csv ---------------------- =========================== short test summary info ============================ FAILED tests/functional/adapter/test_unit_testing.py::TestUnitTestingTypesDuckDB::test_unit_test_data_type - AssertionError: unit test failed when testing model with ARRAY[1,2,3]

How would you feel about me adding @pytest.mark.skip_profile("buenavista") to this test? I know that adds a bit of tech debt / mystery for you later on

I think that’s just because I don’t properly support array types in the BV Postgres serialization, so it’s perfectly understandable and okay to mark it as skipped

jwills · 2024-05-06T16:22:36Z

@jtcohen6 this is all good, right? We're going to cut one last 1.7.x release and then merge this in /cc @guenp

jtcohen6 · 2024-05-06T16:27:40Z

@jwills ready to ship when you see fit!

jtcohen6 added 4 commits February 19, 2024 09:51

Migrate to dbt-common + dbt-adapters

1e25859

Try different install reqs

86eface

Fix unit tests

2d36968

Bonus: functional tests for dbt unit testing

3ad787e

jwills reviewed Feb 23, 2024

View reviewed changes

MichelleArk and others added 4 commits February 29, 2024 17:34

bump dbt-common and dbt-adapters to 1.0.0b1

d4d5939

Merge branch 'master' into jerco/migrate-dbt-common-adapters-interfaces

10034b3

implement DuckDbRelation.create_from, fix TestExternalSources::test_e…

5712711

…xternal_sources

Merge branch 'master' into jerco/migrate-dbt-common-adapters-interfaces

af5f989

MichelleArk mentioned this pull request Mar 15, 2024

SourceDefinition.meta should represent merged base meta and table_meta dbt-labs/dbt-core#9766

Closed

1 task

use RelationConfig attributes in create_from_source

9c8113a

MichelleArk and others added 2 commits March 18, 2024 20:23

Merge branch 'master' into jerco/migrate-dbt-common-adapters-interfaces

80b709d

formatting + initial mypy fixes

b8fdca0

MichelleArk mentioned this pull request Mar 19, 2024

Add "description" and "meta" fields to RelationConfig protocol dbt-labs/dbt-adapters#119

Merged

4 tasks

jtcohen6 added 5 commits April 9, 2024 14:52

Merge branch 'master' into jerco/migrate-dbt-common-adapters-interfaces

6527752

Revert dev-requirements

9a0ba02

Readd dbt-tests-adapter

6dcc39c

Readd dbt-core to setup.py for install back-compat

5f594c4

Merge branch 'master' into jerco/migrate-dbt-common-adapters-interfaces

8511b94

jtcohen6 marked this pull request as ready for review April 16, 2024 10:03

jtcohen6 commented Apr 16, 2024

View reviewed changes

Merge remote-tracking branch 'origin/master' into jerco/migrate-dbt-c…

0750e0c

…ommon-adapters-interfaces

jtcohen6 commented Apr 19, 2024

View reviewed changes

jtcohen6 and others added 2 commits April 19, 2024 15:37

Skip BV for TestUnitTestingTypesDuckDB

a85cef9

Merge branch 'master' into jerco/migrate-dbt-common-adapters-interfaces

298f27c

Merge branch 'master' into jerco/migrate-dbt-common-adapters-interfaces

08d5530

jwills merged commit a42274a into duckdb:master May 8, 2024
31 checks passed

nicholasyager mentioned this pull request May 9, 2024

Confirm support for 1.8.0rc1 nicholasyager/dbt-loom#55

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate to dbt-common + dbt-adapters #342

Migrate to dbt-common + dbt-adapters #342

jtcohen6 commented Feb 19, 2024

jwills left a comment

jwills Feb 23, 2024

jwills Feb 23, 2024

jtcohen6 Feb 26, 2024

MichelleArk Mar 1, 2024 •

edited

Loading

jwills Mar 1, 2024

MichelleArk Mar 18, 2024

jwills Mar 18, 2024

jwills commented Mar 18, 2024

jtcohen6 left a comment

jtcohen6 Apr 16, 2024

jwills commented Apr 16, 2024

jtcohen6 commented Apr 16, 2024

jwills commented Apr 16, 2024

jtcohen6 Apr 19, 2024 •

edited

Loading

jwills Apr 19, 2024

jwills commented May 6, 2024

jtcohen6 commented May 6, 2024

		# add dbt-core to ensure backwards compatibility of installation, this is not a functional dependency
		"dbt-core>=1.8.0b1",

		from dbt.tests.adapter.unit_testing.test_invalid_input import BaseUnitTestInvalidInput


		class TestUnitTestingTypesDuckDB(BaseUnitTestingTypes):

Migrate to dbt-common + dbt-adapters #342

Migrate to dbt-common + dbt-adapters #342

Conversation

jtcohen6 commented Feb 19, 2024

jwills left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichelleArk Mar 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwills commented Mar 18, 2024

jtcohen6 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwills commented Apr 16, 2024

jtcohen6 commented Apr 16, 2024

jwills commented Apr 16, 2024

jtcohen6 Apr 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwills commented May 6, 2024

jtcohen6 commented May 6, 2024

MichelleArk Mar 1, 2024 •

edited

Loading

jtcohen6 Apr 19, 2024 •

edited

Loading