Add support for table comments #308

cbornet · 2023-12-27T12:43:04Z

NB: I didn't find a way to get the comments of temporary views

temporary views don't appear in information_schema
DESCRIBE TABLE EXTENDED doesn't return anything for temporary views
SHOW CREATE TABLE isn't supported for temporary views
SHOW TABLE EXTENDED doesn't report comments for temporary views (it does for default views)
The thrift protocol TGetTablesResp message contains the temporary views but the REMARKS field is empty for all tables

The TCK tests test_comments_unicode and test_comments_unicode_full are still skipped at the moment since they also test column comments which are added in another PR: #306

Signed-off-by: Christophe Bornet <[email protected]>

cbornet · 2024-01-02T00:40:27Z

src/databricks/sqlalchemy/test/_extra.py

+    """This test confirms that a table comment can be dropped.
+    The difference with TableDDLTest is that the comment value is '' and not None after
+    being dropped.
+    """


BTW this is surprising. From the docs we could expect the comment to be set to NULL and not ''.
But that's what I get when I test on databricks cloud.

I adapted for this in #328 by simply coercing the '' value into a NoneType. This passes SQLAlchemy's reusable tests.

src/databricks/sqlalchemy/_information_schema.py

Signed-off-by: Christophe Bornet <[email protected]>

nagyryan · 2024-01-12T22:21:25Z

@susodapop can we get help taking a peek at this one?

NadirJ · 2024-01-23T17:20:34Z

Hello @nagyryan @susodapop @yansonggao-db @andrefurlan-db can one you help @cbornet get this reviewed and merged please? For context @cbornet and I are working on some LangChain apps and require this PR. Ideally we would use an official release instead of using a fork. We know of other devs that in same situation too

NadirJ · 2024-01-23T17:25:07Z

Please let @cbornet or I know if there is anything we can do to move this along

susodapop · 2024-01-23T18:02:50Z

This is being reviewed and tested internally as we need to enable a few e2e test and run the whole suite.

…

On Tue, Jan 23, 2024 at 12:25 PM Nadir J ***@***.***> wrote: Please let @cbornet <https://github.com/cbornet> or I know if there is anything we can do move this along — Reply to this email directly, view it on GitHub <#308 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AECG7B3D2HGQ2XMJYCDWSPTYP7W77AVCNFSM6AAAAABBEKINDGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMBWGU3DEOJWGM> . You are receiving this because you were mentioned.Message ID: ***@***.***>

susodapop · 2024-01-23T20:36:54Z

I've updated the base branch to sqlalchemy-staging so we can incorporate the change in #306 here as well and update the changelog in one go.

[edit] also i merged in sqlalchemy-staging so we have the change from #306 and #328.

Signed-off-by: Jesse Whitehouse <[email protected]>

susodapop

Overall the change code looks great. I'm running all the tests and going to add some unit tests for the statement rendering.

susodapop · 2024-01-23T21:00:51Z

src/databricks/sqlalchemy/_ddl.py

@@ -61,7 +67,7 @@ def get_column_specification(self, column, **kwargs):
        feature in the future, similar to the Microsoft SQL Server dialect.
        """
        if column is column.table._autoincrement_column or column.autoincrement is True:
-            logger.warn(
+            logger.warning(


Good catch 🙏

src/databricks/sqlalchemy/pytest.ini

susodapop · 2024-01-23T21:02:30Z

src/databricks/sqlalchemy/test/_extra.py

+
+    @requirements.comment_reflection
+    @util.provide_metadata
+    def test_drop_table_comment(self, connection):


Since I merged #328 do we still need to override this test?

susodapop · 2024-01-23T21:02:46Z

src/databricks/sqlalchemy/test/_future.py

@@ -251,36 +249,7 @@ class FutureWeCanSetDefaultSchemaWEventsTest(FutureWeCanSetDefaultSchemaWEventsT
    pass


-class FutureTableDDLTest(FutureTableDDLTest):


susodapop · 2024-01-23T21:03:50Z

src/databricks/sqlalchemy/test/_unsupported.py

@@ -55,6 +55,7 @@ class SkipReason(Enum):
    TIMEZONE_OPT = "timezone-optional TIMESTAMP fields"
    TRANSACTIONS = "transactions"
    UNIQUE = "UNIQUE constraints"
+    DROP_TBL = "drop table comment"


Since '' is identical to None as far as sqlalchemy is concerned, I think we needn't list this as unsupported. We can just document the behaviour. Thoughts?

Signed-off-by: Jesse Whitehouse <[email protected]>

class. Scaffold in the Table Comment unit tests. Signed-off-by: Jesse Whitehouse <[email protected]>

Signed-off-by: Jesse Whitehouse <[email protected]>

susodapop · 2024-01-23T21:43:04Z

src/databricks/sqlalchemy/base.py

+            table_name=table_name,
+            schema_name=schema,
+        )
+        # Type ignore is because mypy knows that self._describe_table_extended *can*


We can actually catch this using isinstance to avoid the mypy error like we do in get_pk_constraint.

Signed-off-by: Jesse Whitehouse <[email protected]>

susodapop · 2024-01-23T22:54:27Z

I pushed a few commits making these changes:

I removed the override for TableDDLTest since the OG test now passes.
I added unit tests of the generated SQL for table comments
I moved the logic for parsing table comments out of a DTE result into _parse.py so the logic is consistent.
Added unit tests for item 2
Added an e2e test for using Inspector to get a table comment. This is technically a duplicate of portions of TableDDLTest but I needed it anyway for debugging the new method in _parse.py so might as well keep it.

I concur with your choice not to use the thrift GetTables RPC for this. Eventually we will re-implement a lot of these methods using information_schema but for now, the output of DESCRIBE TABLE EXTENDED is the path forward.

This was missed in databricks#328 Signed-off-by: Jesse Whitehouse <[email protected]>

susodapop · 2024-01-23T23:07:31Z

@cbornet

Alrighty, I have merged this PR into the staging branch and then opened #329 to merge the staging branch into main. I would appreciate you giving that PR a look to make sure everything has been handled to your satisfaction.

cbornet · 2024-01-24T01:16:09Z

Awesome !
Thanks for your review and enhancements (cool solution for the '' comment!).

cbornet requested review from susodapop, arikfr, yunbodeng-db and andrefurlan-db as code owners December 27, 2023 12:43

cbornet force-pushed the table-comment branch from 0e6a2ec to 9c8cbed Compare December 27, 2023 12:52

Add support for table comments

cfdf3f3

Signed-off-by: Christophe Bornet <[email protected]>

cbornet force-pushed the table-comment branch from 9c8cbed to cfdf3f3 Compare January 2, 2024 00:32

cbornet commented Jan 2, 2024

View reviewed changes

src/databricks/sqlalchemy/_information_schema.py Outdated Show resolved Hide resolved

cbornet force-pushed the table-comment branch 2 times, most recently from 5e82562 to e49eed3 Compare January 2, 2024 16:04

Use per-table DTE to get the table comments

ba5ad75

Signed-off-by: Christophe Bornet <[email protected]>

cbornet force-pushed the table-comment branch from e49eed3 to ba5ad75 Compare January 2, 2024 16:09

susodapop mentioned this pull request Jan 23, 2024

Add support for column comments #306

Merged

susodapop changed the base branch from main to sqlalchemy-staging January 23, 2024 20:36

Merge branch 'sqlalchemy-staging' into table-comment

439fe6a

Signed-off-by: Jesse Whitehouse <[email protected]>

susodapop reviewed Jan 23, 2024

View reviewed changes

Jesse Whitehouse added 3 commits January 23, 2024 16:05

Revert pytest.ini change

57b6802

Signed-off-by: Jesse Whitehouse <[email protected]>

Fix typo in test name for columns. Move .engine and .compile into a base

bd68ea8

class. Scaffold in the Table Comment unit tests. Signed-off-by: Jesse Whitehouse <[email protected]>

Add unit tests for table comments

7c94ce3

Signed-off-by: Jesse Whitehouse <[email protected]>

susodapop reviewed Jan 23, 2024

View reviewed changes

Jesse Whitehouse added 4 commits January 23, 2024 17:04

Revert overrides since these aren't needed after databricks#328

c09ae2b

Signed-off-by: Jesse Whitehouse <[email protected]>

Stop skipping table comment portions of ComponentReflectionTest

8f8fef0

Signed-off-by: Jesse Whitehouse <[email protected]>

Move DTE parsing into _parse.py

78a5ad0

Signed-off-by: Jesse Whitehouse <[email protected]>

Add e2e test using inspector

2aa5805

Signed-off-by: Jesse Whitehouse <[email protected]>

Add unit test for new method in _parse.py

f65324f

Signed-off-by: Jesse Whitehouse <[email protected]>

Fix assertion in column comment test

7b6fefb

This was missed in databricks#328 Signed-off-by: Jesse Whitehouse <[email protected]>

susodapop merged commit f41a996 into databricks:sqlalchemy-staging Jan 23, 2024

susodapop mentioned this pull request Jan 23, 2024

[sqlalchemy] Add table and column comment support #329

Merged

cbornet deleted the table-comment branch January 24, 2024 01:14

		@@ -251,36 +249,7 @@ class FutureWeCanSetDefaultSchemaWEventsTest(FutureWeCanSetDefaultSchemaWEventsT
		pass


		class FutureTableDDLTest(FutureTableDDLTest):

Add support for table comments #308

Add support for table comments #308

Uh oh!

Conversation

cbornet commented Dec 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cbornet Jan 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

susodapop Jan 23, 2024

Choose a reason for hiding this comment

Uh oh!

cbornet Jan 24, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nagyryan commented Jan 12, 2024

Uh oh!

NadirJ commented Jan 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NadirJ commented Jan 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

susodapop commented Jan 23, 2024 via email

Uh oh!

susodapop commented Jan 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

susodapop left a comment

Choose a reason for hiding this comment

Uh oh!

susodapop Jan 23, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

susodapop Jan 23, 2024

Choose a reason for hiding this comment

Uh oh!

susodapop Jan 23, 2024

Choose a reason for hiding this comment

Uh oh!

susodapop Jan 23, 2024

Choose a reason for hiding this comment

Uh oh!

susodapop Jan 23, 2024

Choose a reason for hiding this comment

Uh oh!

susodapop commented Jan 23, 2024

Uh oh!

susodapop commented Jan 23, 2024

Uh oh!

cbornet commented Jan 24, 2024

Uh oh!

Uh oh!

cbornet commented Dec 27, 2023 •

edited

Loading

cbornet Jan 2, 2024 •

edited

Loading

NadirJ commented Jan 23, 2024 •

edited

Loading

NadirJ commented Jan 23, 2024 •

edited

Loading

susodapop commented Jan 23, 2024 •

edited

Loading