Convert `information` into `dict` #752

Fokko · 2023-05-08T20:09:19Z

resolves #751

Description

Checklist

I have read the contributing guide and understand what's expected of me
I have signed the CLA
I have run this code in development and it appears to resolve the stated issue
This PR includes tests, or tests are not required/relevant for this PR
I have opened an issue to add/update docs, or docs changes are not required/relevant for this PR
I have run changie new to create a changelog entry

Fokko · 2023-05-09T09:43:15Z

I'll fix the UT's later today

…information-dict

Fokko · 2023-05-10T16:36:35Z

dbt/adapters/spark/impl.py

@@ -139,13 +144,54 @@ def add_schema_to_cache(self, schema) -> str:
    def _get_relation_information(self, row: agate.Row) -> RelationInfo:
        """relation info was fetched with SHOW TABLES EXTENDED"""
        try:
-            _schema, name, _, information = row
+            table_properties = {}


I've changed this by directly separating the columns and properties when we parse the output. I think that makes more sense than splitting this later on because the SHOW TABLES EXTENDED output is different from the DESCRIBE TABLE EXTENDED output.

Fokko · 2023-06-26T07:34:53Z

@mikealfare would you have time to look into this one? :)

dbt/adapters/spark/impl.py

mikealfare · 2023-07-05T19:02:07Z

@mikealfare would you have time to look into this one? :)

This one appears to be a bit more involved and I see that @Fleid labelled this as "pr_tracked". That means it has its own item in our (internal) backlog (= we're aware of it, but it hasn't necessarily been slotted for a sprint yet). I would look to him to get capacity for this issue.

JCZuurmond

I very much like the goal of this PR: converting the information string to a columns and properties dict!

But ... the code changes are lot. If we can test it well and with the suggested changes, I think it is the good enough to move forward to the features that benefit from the columns and properties dict.

JCZuurmond · 2023-07-07T13:16:07Z

tests/unit/test_adapter.py

@@ -320,10 +322,15 @@ def test_parse_relation(self):
        input_cols = [Row(keys=["col_name", "data_type"], values=r) for r in plain_rows]

        config = self._get_target_http(self.project_cfg)
-        rows = SparkAdapter(config).parse_describe_extended(relation, input_cols)
-        self.assertEqual(len(rows), 4)
+        adapter = SparkAdapter(config)


@Fokko : Could you rewrite this code to have a similar flow as before, i.e. (re)move the three build-up lines and have one call to get the to-be-tested object

I've condensed the code quite a bit. Let me know what you think!

JCZuurmond · 2023-07-07T13:16:50Z

dbt/adapters/spark/relation.py

@@ -33,8 +33,8 @@ class SparkRelation(BaseRelation):
    is_delta: Optional[bool] = None
    is_hudi: Optional[bool] = None
    is_iceberg: Optional[bool] = None
-    # TODO: make this a dict everywhere
-    information: Optional[str] = None
+    columns: List[Tuple[str, str]] = field(default_factory=list)


I like this! Much better than the information string ❤️

JCZuurmond · 2023-07-07T13:18:34Z

dbt/adapters/spark/impl.py

@@ -138,13 +143,54 @@ def quote(self, identifier: str) -> str:  # type: ignore
    def _get_relation_information(self, row: agate.Row) -> RelationInfo:
        """relation info was fetched with SHOW TABLES EXTENDED"""


@Fokko : Could you extend the docstring to explain that the SHOW TABLES EXTENDED is preferred because fetching multiple tables at once is faster than fetching tables one by one. And, that we except the downside of parsing the |--- string given the performance gains?

Certainly. I've added a docstring. Let me know what you think

JCZuurmond · 2023-07-07T13:18:46Z

dbt/adapters/spark/impl.py

+        self, table_results: agate.Table
+    ) -> Tuple[List[Tuple[str, str]], Dict[str, str]]:
+        # Wrap it in an iter, so we continue reading the properties from where we stopped reading columns
+        table_results_itr = iter(table_results)


dbt/adapters/spark/impl.py

Co-authored-by: Cor <[email protected]>

Fokko

Uh oh, the comment was still pending

Fokko · 2023-07-07T13:48:41Z

tests/unit/test_adapter.py

@@ -320,10 +322,15 @@ def test_parse_relation(self):
        input_cols = [Row(keys=["col_name", "data_type"], values=r) for r in plain_rows]

        config = self._get_target_http(self.project_cfg)
-        rows = SparkAdapter(config).parse_describe_extended(relation, input_cols)
-        self.assertEqual(len(rows), 4)
+        adapter = SparkAdapter(config)


I've condensed the code quite a bit. Let me know what you think!

Fokko · 2023-07-07T13:51:51Z

dbt/adapters/spark/impl.py

@@ -138,13 +143,54 @@ def quote(self, identifier: str) -> str:  # type: ignore
    def _get_relation_information(self, row: agate.Row) -> RelationInfo:
        """relation info was fetched with SHOW TABLES EXTENDED"""


Certainly. I've added a docstring. Let me know what you think

github-actions · 2024-02-07T01:43:00Z

This PR has been marked as Stale because it has been open with no activity as of late. If you would like the PR to remain open, please comment on the PR or else it will be closed in 7 days.

github-actions · 2024-08-06T01:50:34Z

This PR has been marked as Stale because it has been open with no activity as of late. If you would like the PR to remain open, please comment on the PR or else it will be closed in 7 days.

Fokko requested a review from a team as a code owner May 8, 2023 20:09

Fokko requested a review from nssalian May 8, 2023 20:09

cla-bot bot added the cla:yes label May 8, 2023

Fokko force-pushed the fd-information-dict branch 2 times, most recently from 17ad88f to af5ea1f Compare May 8, 2023 20:11

Fokko changed the title ~~Convert information into dict~~ Convert information into dict May 8, 2023

Convert information into dict

5e6527c

Fokko force-pushed the fd-information-dict branch from af5ea1f to 5e6527c Compare May 8, 2023 20:27

Fokko added 3 commits May 9, 2023 15:25

Merge branch 'main' of https://github.com/dbt-labs/dbt-spark into fd-…

d21fe88

…information-dict

Fix and add a test

6cb4289

Cleanup and tests

a423803

Fokko force-pushed the fd-information-dict branch 3 times, most recently from 457b311 to 723729c Compare May 9, 2023 20:23

Cleanup

036f278

Fokko force-pushed the fd-information-dict branch from 723729c to 036f278 Compare May 9, 2023 20:28

Fleid mentioned this pull request May 9, 2023

[ADAP-528] [PR Tracking] Convert information into dict #752 #756

Closed

Fleid added the pr_tracked label May 9, 2023

Fokko force-pushed the fd-information-dict branch 5 times, most recently from bc48118 to 27aef99 Compare May 10, 2023 09:54

Improve error reporting

f697b80

Fokko force-pushed the fd-information-dict branch 2 times, most recently from 61d68fa to 06cfeda Compare May 10, 2023 14:45

Fokko commented May 10, 2023

View reviewed changes

Cleanup

f97c161

Fokko force-pushed the fd-information-dict branch from 06cfeda to f97c161 Compare May 10, 2023 16:48

Fokko added 9 commits May 14, 2023 21:48

Merge branch 'main' into fd-information-dict

e1b74ff

Merge branch 'main' into fd-information-dict

e14d512

Merge branch 'main' into fd-information-dict

be0a387

Merge branch 'main' into fd-information-dict

2612f98

Merge branch 'main' into fd-information-dict

4a421e3

Merge branch 'main' into fd-information-dict

639a663

Merge branch 'main' into fd-information-dict

0849382

Merge branch 'main' into fd-information-dict

13cda76

Merge branch 'main' into fd-information-dict

f1b9034

Fokko added 2 commits June 26, 2023 22:10

Merge branch 'main' into fd-information-dict

4b5c45a

Merge branch 'main' into fd-information-dict

3435f2e

Fokko commented Jun 29, 2023

View reviewed changes

dbt/adapters/spark/impl.py Outdated Show resolved Hide resolved

Update dbt/adapters/spark/impl.py

85395df

mikealfare removed the request for review from nssalian July 5, 2023 18:56

JCZuurmond suggested changes Jul 7, 2023

View reviewed changes

Fokko and others added 2 commits August 4, 2023 16:24

Merge branch 'main' into fd-information-dict

cc5e536

Update dbt/adapters/spark/impl.py

7a9c50b

Co-authored-by: Cor <[email protected]>

Fokko commented Aug 7, 2023

View reviewed changes

Fokko added 2 commits August 7, 2023 14:18

Merge branch 'main' into fd-information-dict

9182ee9

Merge branch 'main' into fd-information-dict

c7d39d2

github-actions bot added the Stale label Feb 7, 2024

mikealfare removed the pr_tracked label Feb 7, 2024

github-actions bot removed the Stale label Feb 8, 2024

github-actions bot added the Stale label Aug 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert `information` into `dict` #752

Convert `information` into `dict` #752

Fokko commented May 8, 2023 •

edited

Loading

Fokko commented May 9, 2023

Fokko May 10, 2023

Fokko commented Jun 26, 2023

mikealfare commented Jul 5, 2023

JCZuurmond left a comment

JCZuurmond Jul 7, 2023

Fokko Jul 7, 2023

JCZuurmond Jul 7, 2023

JCZuurmond Jul 7, 2023

Fokko Jul 7, 2023

JCZuurmond Jul 7, 2023

Fokko left a comment

Fokko Jul 7, 2023

Fokko Jul 7, 2023

github-actions bot commented Feb 7, 2024

github-actions bot commented Aug 6, 2024

		@@ -138,13 +143,54 @@ def quote(self, identifier: str) -> str: # type: ignore
		def _get_relation_information(self, row: agate.Row) -> RelationInfo:
		"""relation info was fetched with SHOW TABLES EXTENDED"""

Convert information into dict #752

Are you sure you want to change the base?

Convert information into dict #752

Conversation

Fokko commented May 8, 2023 • edited Loading

Description

Checklist

Fokko commented May 9, 2023

Choose a reason for hiding this comment

Fokko commented Jun 26, 2023

mikealfare commented Jul 5, 2023

JCZuurmond 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fokko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 7, 2024

github-actions bot commented Aug 6, 2024

Convert `information` into `dict` #752

Convert `information` into `dict` #752

Fokko commented May 8, 2023 •

edited

Loading