[5/n][dagster-fivetran] Implement `FivetranWorkspaceData` to `FivetranConnectorTableProps` method #25797

maximearmstrong · 2024-11-07T22:22:51Z

Summary & Motivation

This PR implements FivetranWorkspaceData.to_fivetran_connector_table_props_data(), a method that converts a FivetranWorkspaceData object to a list of FivetranConnectorTableProps.

To create the asset spec, we need one FivetranConnectorTableProps object per connector table. This method parses the API raw data to create the FivetranConnectorTablePropsobject, which is compatible with theDagsterFivetranTranslator`.

This will be used in the defs_from_state method of the stated-backed defs loader in a subsequent PR.

How I Tested These Changes

Additional unit test

maximearmstrong · 2024-11-07T22:23:01Z

This stack of pull requests is managed by Graphite. Learn more about stacking.

dpeng817

some more design stuff

dpeng817 · 2024-11-08T17:05:40Z

...on_modules/libraries/dagster-fivetran/dagster_fivetran_tests/experimental/test_translator.py

@@ -0,0 +1,30 @@
+import uuid


nit; why is this in translator tests? Shouldn't it be in the same place as the previous workspace method tests?

Done in 697e460

dpeng817 · 2024-11-08T17:21:09Z

python_modules/libraries/dagster-fivetran/dagster_fivetran/translator.py

-        raise NotImplementedError()
+        data: List[FivetranConnectorTableProps] = []
+
+        for connector_id, connector_data in self.connectors_by_id.items():


In airlift, from the raw API data we retrieved, we constructed a "lookup" object which built up the actual usable asset data from a set of cacheable properties. See

dagster/examples/experimental/dagster-airlift/dagster_airlift/core/serialization/compute.py

Line 114 in 0a0c77b

class FetchedAirflowData:

for an example of what I mean.

Since this is over the "cacheable" boundary, I'm wondering if there's a likelihood if it being called a bunch, and if so I think the cacheable structure likely makes sense. At the very least, I think that the conversion to FivetranConnectorTableProps should probably be cached, but it might also make sense to cache the stuff from 73-83 here as properties if there's potential for reuse (but if not, then maybe that's not worth the effort)

That's a good point. This method is meant to be used only in FivetranWorkspaceDefsLoader.defs_from_state, see implement in next PR #25807.

But I think caching it makes sense.

I cached the method in b272d07

dpeng817 · 2024-11-08T17:21:40Z

python_modules/libraries/dagster-fivetran/dagster_fivetran/translator.py

+            for schema in schemas_data.values():
+                if schema["enabled"]:
+                    schema_name = schema["name_in_destination"]
+                    schema_tables: Dict[str, Dict[str, Any]] = cast(


strong code smell from this one. The complexity from the previous PR feels like it's spilling over into this one with all the casting and raw string munging we need to do. Let's strongly type all of these objects, I think things will feel much better.

Agreed. This comment applies here as well - this is taken from the legacy code to replicate the behavior. It will be updated in the same PR refactoring how we are storing destinations and connectors.

dpeng817 · 2024-11-08T17:22:37Z

python_modules/libraries/dagster-fivetran/dagster_fivetran/translator.py

+                            table_name = table["name_in_destination"]
+                            data.append(
+                                FivetranConnectorTableProps(
+                                    table=f"{schema_name}.{table_name}",


This feels like it should be a standalone method / something reusable so that we're consistent about how it's constructed.

Done in 0040d59

dpeng817 · 2024-11-08T17:22:46Z

python_modules/libraries/dagster-fivetran/dagster_fivetran/translator.py

@@ -80,7 +117,7 @@ def get_asset_spec(self, props: FivetranConnectorTableProps) -> AssetSpec:
        schema_name, table_name = props.table.split(".")
        schema_entry = next(
            schema
-            for schema in props.schemas["schemas"].values()
+            for schema in props.schema_config["schemas"].values()


why the name change?

To reflect Fivetran's ontology, see here

re-requesting

dpeng817

nits for your consideration.

dpeng817 · 2024-11-12T17:14:51Z

python_modules/libraries/dagster-fivetran/dagster_fivetran_tests/experimental/conftest.py

@@ -381,6 +382,16 @@
 }


+@pytest.fixture(name="api_key")
+def api_key_fixture() -> str:


nit; I feel like this makes more sense to be a constant than a fixture, personally. I feel like when I see a mountain of fixtures, I get overwhelmed by a test. I know that this doesn't originate with this PR, but I personally prefer TEST_API_KEY, TEST_API_SECRET, etc

That's fair - done in ad9fd4e

dpeng817 · 2024-11-12T17:15:18Z

python_modules/libraries/dagster-fivetran/dagster_fivetran_tests/experimental/test_resources.py

+    api_key: str,
+    api_secret: str,
    connector_id: str,
    destination_id: str,
    group_id: str,


yea these all feel like they could be constants and then the test feels a little less overwhelming, lol.

…nectorTableProps method

This was referenced Nov 7, 2024

[1/n][dagster-fivetran] Scaffold FivetranWorkspace for rework #25750

Merged

[2/n][dagster-fivetran] Update DagsterFivetranTranslator and related classes for rework #25751

Merged

[3/n][dagster-fivetran] Implement FivetranClient for rework #25756

Merged

maximearmstrong mentioned this pull request Nov 7, 2024

[4/n][dagster-fivetran] Implement fetch_fivetran_workspace_data #25788

Merged

maximearmstrong force-pushed the maxime/rework-fivetran-4 branch from b17bd45 to 252be0c Compare November 8, 2024 14:02

maximearmstrong force-pushed the maxime/rework-fivetran-5 branch from 19493b4 to d5a03b3 Compare November 8, 2024 14:02

maximearmstrong changed the title ~~[5/n][dagster-fivetran] Implement FivetranWorkspaceData to FivetranConnectorTableProps method~~ [5/n][dagster-fivetran] Implement FivetranWorkspaceData to FivetranConnectorTableProps method Nov 8, 2024

maximearmstrong marked this pull request as ready for review November 8, 2024 14:20

maximearmstrong requested review from benpankow and dpeng817 November 8, 2024 14:20

maximearmstrong force-pushed the maxime/rework-fivetran-4 branch from 252be0c to 56205b0 Compare November 8, 2024 16:44

maximearmstrong force-pushed the maxime/rework-fivetran-5 branch from 1a573eb to 16e3497 Compare November 8, 2024 16:44

This was referenced Nov 8, 2024

[6/n][dagster-fivetran] Implement FivetranWorkspaceDefsLoader #25807

Merged

[7/n][dagster-fivetran] Implement load_fivetran_asset_specs #25808

Merged

maximearmstrong force-pushed the maxime/rework-fivetran-4 branch from 56205b0 to 6c50270 Compare November 8, 2024 17:00

maximearmstrong force-pushed the maxime/rework-fivetran-5 branch from 16e3497 to aad712a Compare November 8, 2024 17:00

dpeng817 previously requested changes Nov 8, 2024

View reviewed changes

maximearmstrong force-pushed the maxime/rework-fivetran-4 branch from 6c50270 to 55248a3 Compare November 8, 2024 17:41

maximearmstrong force-pushed the maxime/rework-fivetran-5 branch from aad712a to dc9a490 Compare November 8, 2024 17:41

maximearmstrong force-pushed the maxime/rework-fivetran-4 branch from 55248a3 to 5c21300 Compare November 8, 2024 19:45

maximearmstrong force-pushed the maxime/rework-fivetran-5 branch from dc9a490 to cfcd238 Compare November 8, 2024 19:45

maximearmstrong force-pushed the maxime/rework-fivetran-4 branch from 5c21300 to 011989c Compare November 8, 2024 20:47

maximearmstrong force-pushed the maxime/rework-fivetran-5 branch from cfcd238 to 916f3ea Compare November 8, 2024 20:47

maximearmstrong force-pushed the maxime/rework-fivetran-4 branch from 011989c to 71bf953 Compare November 11, 2024 20:14

maximearmstrong force-pushed the maxime/rework-fivetran-5 branch from 916f3ea to 1d5bf36 Compare November 11, 2024 20:15

maximearmstrong requested a review from dpeng817 November 11, 2024 20:16

maximearmstrong force-pushed the maxime/rework-fivetran-4 branch from 71bf953 to 7dd39c7 Compare November 11, 2024 22:21

maximearmstrong force-pushed the maxime/rework-fivetran-5 branch from 1d5bf36 to c407973 Compare November 11, 2024 22:21

dpeng817 approved these changes Nov 12, 2024

View reviewed changes

Base automatically changed from maxime/rework-fivetran-4 to master November 12, 2024 18:11

maximearmstrong added 7 commits November 12, 2024 13:20

[5/n][dagster-tableau] Implement FivetranWorkspaceData to FivetranCon…

6b69e7b

…nectorTableProps method

Update asset defs

f9183c1

Fix tests

772f724

Cache to_fivetran_connector_table_props_data

2da88a9

Add get_fivetran_connector_table_name to utils

b23abe4

Move api_key and api_secret mocks to conftest

3792701

Lint

4913f6e

maximearmstrong force-pushed the maxime/rework-fivetran-5 branch from c407973 to 4913f6e Compare November 12, 2024 18:20

Update tests

ad9fd4e

maximearmstrong merged commit d6f4f52 into master Nov 12, 2024
1 check passed

maximearmstrong deleted the maxime/rework-fivetran-5 branch November 12, 2024 20:01

maximearmstrong mentioned this pull request Nov 27, 2024

[dagster-fivetran] Implement get_columns_config_for_table in FivetranClient #26181

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[5/n][dagster-fivetran] Implement `FivetranWorkspaceData` to `FivetranConnectorTableProps` method #25797

[5/n][dagster-fivetran] Implement `FivetranWorkspaceData` to `FivetranConnectorTableProps` method #25797

maximearmstrong commented Nov 7, 2024 •

edited

Loading

maximearmstrong commented Nov 7, 2024 •

edited

Loading

dpeng817 left a comment

dpeng817 Nov 8, 2024

maximearmstrong Nov 11, 2024

dpeng817 Nov 8, 2024

maximearmstrong Nov 11, 2024 •

edited

Loading

maximearmstrong Nov 11, 2024

dpeng817 Nov 8, 2024

maximearmstrong Nov 8, 2024

dpeng817 Nov 8, 2024

maximearmstrong Nov 11, 2024

dpeng817 Nov 8, 2024

maximearmstrong Nov 8, 2024

dpeng817 left a comment

dpeng817 Nov 12, 2024

maximearmstrong Nov 12, 2024

dpeng817 Nov 12, 2024

[5/n][dagster-fivetran] Implement FivetranWorkspaceData to FivetranConnectorTableProps method #25797

[5/n][dagster-fivetran] Implement FivetranWorkspaceData to FivetranConnectorTableProps method #25797

Conversation

maximearmstrong commented Nov 7, 2024 • edited Loading

Summary & Motivation

How I Tested These Changes

maximearmstrong commented Nov 7, 2024 • edited Loading

dpeng817 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

maximearmstrong Nov 11, 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

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

dpeng817 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

[5/n][dagster-fivetran] Implement `FivetranWorkspaceData` to `FivetranConnectorTableProps` method #25797

[5/n][dagster-fivetran] Implement `FivetranWorkspaceData` to `FivetranConnectorTableProps` method #25797

maximearmstrong commented Nov 7, 2024 •

edited

Loading

maximearmstrong commented Nov 7, 2024 •

edited

Loading

maximearmstrong Nov 11, 2024 •

edited

Loading