From 726b05d96bccf4e7c2411f8e0bd1d23b51e58618 Mon Sep 17 00:00:00 2001 From: dotanmor Date: Sun, 8 Sep 2024 20:28:50 +0300 Subject: [PATCH 1/5] add column comment to get_columns method --- sqlalchemy_cockroachdb/base.py | 8 +++++--- test/test_column_reflect.py | 5 +++++ 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/sqlalchemy_cockroachdb/base.py b/sqlalchemy_cockroachdb/base.py index acb433d..49b6ba2 100644 --- a/sqlalchemy_cockroachdb/base.py +++ b/sqlalchemy_cockroachdb/base.py @@ -182,9 +182,10 @@ def get_columns(self, conn, table_name, schema=None, **kw): if not self._is_v191plus: # v2.x does not have is_generated or generation_expression sql = ( - "SELECT column_name, data_type, is_nullable::bool, column_default, " + "SELECT column_name, data_type, is_nullable::bool, column_default," "numeric_precision, numeric_scale, character_maximum_length, " - "NULL AS is_generated, NULL AS generation_expression, is_hidden::bool " + "NULL AS is_generated, NULL AS generation_expression, is_hidden::bool," + "comment " "FROM information_schema.columns " "WHERE table_schema = :table_schema AND table_name = :table_name " ) @@ -200,7 +201,7 @@ def get_columns(self, conn, table_name, schema=None, **kw): "numeric_precision, numeric_scale, character_maximum_length, " "CASE is_generated WHEN 'ALWAYS' THEN true WHEN 'NEVER' THEN false " "ELSE is_generated::bool END AS is_generated, " - "generation_expression, is_hidden::bool, crdb_sql_type " + "generation_expression, is_hidden::bool, crdb_sql_type, comment " "FROM information_schema.columns " "WHERE table_schema = :table_schema AND table_name = :table_name " ) @@ -281,6 +282,7 @@ def get_columns(self, conn, table_name, schema=None, **kw): default=default, autoincrement=autoincrement, is_hidden=row.is_hidden, + comment=row.comment, ) if computed is not None: column_info["computed"] = computed diff --git a/test/test_column_reflect.py b/test/test_column_reflect.py index 032b3f7..4853cdb 100644 --- a/test/test_column_reflect.py +++ b/test/test_column_reflect.py @@ -44,6 +44,7 @@ def test_reflect_hidden_columns(self): "default": "unique_rowid()", "autoincrement": True, "is_hidden": False, + "comment": None, }, { "name": "txt", @@ -52,6 +53,7 @@ def test_reflect_hidden_columns(self): "default": None, "autoincrement": False, "is_hidden": False, + "comment": None, }, ], ) @@ -66,6 +68,7 @@ def test_reflect_hidden_columns(self): "default": None, "autoincrement": False, "is_hidden": False, + "comment": None, }, ], ) @@ -80,6 +83,7 @@ def test_reflect_hidden_columns(self): "default": None, "autoincrement": False, "is_hidden": False, + "comment": None, }, { "name": "rowid", @@ -88,6 +92,7 @@ def test_reflect_hidden_columns(self): "default": "unique_rowid()", "autoincrement": True, "is_hidden": True, + "comment": None, }, ], ) From ef4ff2db5aea36ebe3282bd274678d1778d7fd65 Mon Sep 17 00:00:00 2001 From: dotanmor Date: Mon, 9 Sep 2024 18:19:45 +0300 Subject: [PATCH 2/5] add column comment to get_columns method --- sqlalchemy_cockroachdb/base.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sqlalchemy_cockroachdb/base.py b/sqlalchemy_cockroachdb/base.py index 49b6ba2..19f8a97 100644 --- a/sqlalchemy_cockroachdb/base.py +++ b/sqlalchemy_cockroachdb/base.py @@ -185,7 +185,7 @@ def get_columns(self, conn, table_name, schema=None, **kw): "SELECT column_name, data_type, is_nullable::bool, column_default," "numeric_precision, numeric_scale, character_maximum_length, " "NULL AS is_generated, NULL AS generation_expression, is_hidden::bool," - "comment " + "column_comment AS comment " "FROM information_schema.columns " "WHERE table_schema = :table_schema AND table_name = :table_name " ) @@ -201,7 +201,7 @@ def get_columns(self, conn, table_name, schema=None, **kw): "numeric_precision, numeric_scale, character_maximum_length, " "CASE is_generated WHEN 'ALWAYS' THEN true WHEN 'NEVER' THEN false " "ELSE is_generated::bool END AS is_generated, " - "generation_expression, is_hidden::bool, crdb_sql_type, comment " + "generation_expression, is_hidden::bool, crdb_sql_type, column_comment AS comment " "FROM information_schema.columns " "WHERE table_schema = :table_schema AND table_name = :table_name " ) From 37510426d5eb04781f38d7f72f18b77fad30324e Mon Sep 17 00:00:00 2001 From: Gord Thompson Date: Mon, 9 Sep 2024 12:23:52 -0600 Subject: [PATCH 3/5] Update test_get_multi_columns --- sqlalchemy_cockroachdb/base.py | 2 +- test/test_suite_sqlalchemy.py | 243 ++++++++++++++++++--------------- 2 files changed, 131 insertions(+), 114 deletions(-) diff --git a/sqlalchemy_cockroachdb/base.py b/sqlalchemy_cockroachdb/base.py index 19f8a97..61ff0a0 100644 --- a/sqlalchemy_cockroachdb/base.py +++ b/sqlalchemy_cockroachdb/base.py @@ -86,7 +86,7 @@ def __init__(self): class CockroachDBDialect(PGDialect): name = "cockroachdb" - supports_comments = False + supports_comments = True supports_empty_insert = True supports_multivalues_insert = True supports_sequences = False diff --git a/test/test_suite_sqlalchemy.py b/test/test_suite_sqlalchemy.py index e8e00ec..afaa87e 100644 --- a/test/test_suite_sqlalchemy.py +++ b/test/test_suite_sqlalchemy.py @@ -4,14 +4,20 @@ from sqlalchemy.testing.suite import ( BizarroCharacterFKResolutionTest as _BizarroCharacterFKResolutionTest, ) -from sqlalchemy.testing.suite import ComponentReflectionTest as _ComponentReflectionTest +from sqlalchemy.testing.suite import ( + ComponentReflectionTest as _ComponentReflectionTest, +) from sqlalchemy.testing.suite import HasIndexTest as _HasIndexTest from sqlalchemy.testing.suite import HasTableTest as _HasTableTest from sqlalchemy.testing.suite import InsertBehaviorTest as _InsertBehaviorTest from sqlalchemy.testing.suite import IsolationLevelTest as _IsolationLevelTest -from sqlalchemy.testing.suite import LongNameBlowoutTest as _LongNameBlowoutTest +from sqlalchemy.testing.suite import ( + LongNameBlowoutTest as _LongNameBlowoutTest, +) from sqlalchemy.testing.suite import NumericTest as _NumericTest -from sqlalchemy.testing.suite import QuotedNameArgumentTest as _QuotedNameArgumentTest +from sqlalchemy.testing.suite import ( + QuotedNameArgumentTest as _QuotedNameArgumentTest, +) from sqlalchemy.testing.suite import TrueDivTest as _TrueDivTest from sqlalchemy.testing.suite import UnicodeSchemaTest as _UnicodeSchemaTest @@ -27,17 +33,13 @@ class BizarroCharacterFKResolutionTest(_BizarroCharacterFKResolutionTest): argnames="tablename", ) def test_fk_ref(self, connection, metadata, use_composite, tablename, columnname): - if not ( - config.db.dialect.driver == "asyncpg" and not config.db.dialect._is_v231plus - ): + if not (config.db.dialect.driver == "asyncpg" and not config.db.dialect._is_v231plus): super().test_fk_ref(connection, metadata, use_composite, tablename, columnname) class ComponentReflectionTest(_ComponentReflectionTest): def test_get_indexes(self, connection): - if not ( - config.db.dialect.driver == "asyncpg" and not config.db.dialect._is_v231plus - ): + if not (config.db.dialect.driver == "asyncpg" and not config.db.dialect._is_v231plus): super().test_get_indexes(connection, None, None) @skip("cockroachdb") @@ -49,176 +51,201 @@ def test_get_multi_columns(self): insp = inspect(config.db) actual = insp.get_multi_columns() expected = { - (None, "comment_test"): [ + (None, "users"): [ { - "autoincrement": True, + "name": "user_id", + "type": INTEGER(), + "nullable": False, "default": "unique_rowid()", + "autoincrement": True, "is_hidden": False, - "name": "id", - "nullable": False, - "type": INTEGER(), + "comment": None, }, { - "autoincrement": False, + "name": "test1", + "type": VARCHAR(length=5), + "nullable": False, "default": None, + "autoincrement": False, "is_hidden": False, - "name": "data", - "nullable": True, - "type": VARCHAR(length=20), + "comment": None, }, { - "autoincrement": False, + "name": "test2", + "type": FLOAT(), + "nullable": False, "default": None, + "autoincrement": False, "is_hidden": False, - "name": "d2", - "nullable": True, - "type": VARCHAR(length=20), + "comment": None, }, { - "autoincrement": False, + "name": "parent_user_id", + "type": INTEGER(), + "nullable": True, "default": None, + "autoincrement": False, "is_hidden": False, - "name": "d3", - "nullable": True, - "type": VARCHAR(length=42), + "comment": None, }, ], - (None, "dingalings"): [ + (None, "comment_test"): [ { - "autoincrement": True, + "name": "id", + "type": INTEGER(), + "nullable": False, "default": "unique_rowid()", + "autoincrement": True, "is_hidden": False, - "name": "dingaling_id", - "nullable": False, - "type": INTEGER(), + "comment": "id comment", }, { - "autoincrement": False, - "default": None, - "is_hidden": False, - "name": "address_id", + "name": "data", + "type": VARCHAR(length=20), "nullable": True, - "type": INTEGER(), - }, - { - "autoincrement": False, "default": None, - "is_hidden": False, - "name": "id_user", - "nullable": True, - "type": INTEGER(), - }, - { "autoincrement": False, - "default": None, - "is_hidden": False, - "name": "data", - "nullable": True, - "type": VARCHAR(length=30), - }, - ], - (None, "email_addresses"): [ - { - "autoincrement": True, - "default": "unique_rowid()", "is_hidden": False, - "name": "address_id", - "nullable": False, - "type": INTEGER(), + "comment": "data % comment", }, { - "autoincrement": False, + "name": "d2", + "type": VARCHAR(length=20), + "nullable": True, "default": None, + "autoincrement": False, "is_hidden": False, - "name": "remote_user_id", - "nullable": True, - "type": INTEGER(), + "comment": "Comment types type speedily ' \" \\ '' Fun!", }, { - "autoincrement": False, + "name": "d3", + "type": VARCHAR(length=42), + "nullable": True, "default": None, + "autoincrement": False, "is_hidden": False, - "name": "email_address", - "nullable": True, - "type": VARCHAR(length=20), + "comment": "Comment\nwith\rescapes", }, ], (None, "no_constraints"): [ { - "autoincrement": False, - "default": None, - "is_hidden": False, "name": "data", - "nullable": True, "type": VARCHAR(length=20), + "nullable": True, + "default": None, + "autoincrement": False, + "is_hidden": False, + "comment": None, } ], (None, "noncol_idx_test_nopk"): [ { - "autoincrement": False, - "default": None, - "is_hidden": False, "name": "q", - "nullable": True, "type": VARCHAR(length=5), + "nullable": True, + "default": None, + "autoincrement": False, + "is_hidden": False, + "comment": None, } ], (None, "noncol_idx_test_pk"): [ { - "autoincrement": True, - "default": "unique_rowid()", - "is_hidden": False, "name": "id", - "nullable": False, "type": INTEGER(), + "nullable": False, + "default": "unique_rowid()", + "autoincrement": True, + "is_hidden": False, + "comment": None, }, { - "autoincrement": False, - "default": None, - "is_hidden": False, "name": "q", - "nullable": True, "type": VARCHAR(length=5), + "nullable": True, + "default": None, + "autoincrement": False, + "is_hidden": False, + "comment": None, }, ], - (None, "users"): [ + (None, "email_addresses"): [ { - "autoincrement": True, + "name": "address_id", + "type": INTEGER(), + "nullable": False, "default": "unique_rowid()", + "autoincrement": True, "is_hidden": False, - "name": "user_id", - "nullable": False, - "type": INTEGER(), + "comment": None, }, { - "autoincrement": False, + "name": "remote_user_id", + "type": INTEGER(), + "nullable": True, "default": None, + "autoincrement": False, "is_hidden": False, - "name": "test1", - "nullable": False, - "type": VARCHAR(length=5), + "comment": None, }, { - "autoincrement": False, + "name": "email_address", + "type": VARCHAR(length=20), + "nullable": True, "default": None, + "autoincrement": False, "is_hidden": False, - "name": "test2", + "comment": None, + }, + ], + (None, "dingalings"): [ + { + "name": "dingaling_id", + "type": INTEGER(), "nullable": False, - "type": FLOAT(), + "default": "unique_rowid()", + "autoincrement": True, + "is_hidden": False, + "comment": None, }, { + "name": "address_id", + "type": INTEGER(), + "nullable": True, + "default": None, "autoincrement": False, + "is_hidden": False, + "comment": None, + }, + { + "name": "id_user", + "type": INTEGER(), + "nullable": True, "default": None, + "autoincrement": False, "is_hidden": False, - "name": "parent_user_id", + "comment": None, + }, + { + "name": "data", + "type": VARCHAR(length=30), "nullable": True, - "type": INTEGER(), + "default": None, + "autoincrement": False, + "is_hidden": False, + "comment": None, }, ], } eq_(len(actual), len(expected)) eq_(actual.keys(), expected.keys()) - eq_(len(actual[(None, "comment_test")]), len(expected[(None, "comment_test")])) + eq_( + len(actual[(None, "comment_test")]), + len(expected[(None, "comment_test")]), + ) + act = [x for x in actual[(None, "comment_test")] if x["name"] == "data"][0] + exp = [x for x in expected[(None, "comment_test")] if x["name"] == "data"][0] + eq_(act["comment"], exp["comment"]) @skip("cockroachdb") def test_get_multi_indexes(self): @@ -244,13 +271,9 @@ def test_get_view_names(self): pass @testing.combinations(True, False, argnames="use_schema") - @testing.combinations( - (True, testing.requires.views), False, argnames="views" - ) + @testing.combinations((True, testing.requires.views), False, argnames="views") def test_metadata(self, connection, use_schema, views): - if not ( - config.db.dialect.driver == "asyncpg" and not config.db.dialect._is_v231plus - ): + if not (config.db.dialect.driver == "asyncpg" and not config.db.dialect._is_v231plus): super().test_metadata(connection, use_schema, views, []) @skip("cockroachdb") @@ -310,9 +333,7 @@ class LongNameBlowoutTest(_LongNameBlowoutTest): argnames="type_", ) def test_long_convention_name(self, type_, metadata, connection): - if not ( - config.db.dialect.driver == "asyncpg" and not config.db.dialect._is_v231plus - ): + if not (config.db.dialect.driver == "asyncpg" and not config.db.dialect._is_v231plus): super().test_long_convention_name(type_, metadata, connection, None) @@ -326,9 +347,7 @@ def quote_fixtures(fn): @quote_fixtures def test_get_indexes(self, name): # could not decorrelate subquery - if not ( - config.db.dialect.driver == "asyncpg" and not config.db.dialect._is_v231plus - ): + if not (config.db.dialect.driver == "asyncpg" and not config.db.dialect._is_v231plus): super().test_get_indexes(name, None) @@ -358,7 +377,5 @@ def test_floordiv_integer_bound(self): class UnicodeSchemaTest(_UnicodeSchemaTest): def test_reflect(self, connection): - if not ( - config.db.dialect.driver == "asyncpg" and not config.db.dialect._is_v231plus - ): + if not (config.db.dialect.driver == "asyncpg" and not config.db.dialect._is_v231plus): super().test_reflect(connection) From af5fa9f73f0a0c3cf36257ffea4ff45375a95cff Mon Sep 17 00:00:00 2001 From: Gord Thompson Date: Mon, 9 Sep 2024 12:44:55 -0600 Subject: [PATCH 4/5] Revert change to supports_comments (Alembic doesn't like it.) --- sqlalchemy_cockroachdb/base.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sqlalchemy_cockroachdb/base.py b/sqlalchemy_cockroachdb/base.py index 61ff0a0..19f8a97 100644 --- a/sqlalchemy_cockroachdb/base.py +++ b/sqlalchemy_cockroachdb/base.py @@ -86,7 +86,7 @@ def __init__(self): class CockroachDBDialect(PGDialect): name = "cockroachdb" - supports_comments = True + supports_comments = False supports_empty_insert = True supports_multivalues_insert = True supports_sequences = False From aff2a6ccbbeedcdf7cdd89a4cefda6e36c8c5d54 Mon Sep 17 00:00:00 2001 From: Gord Thompson Date: Mon, 9 Sep 2024 13:13:27 -0600 Subject: [PATCH 5/5] Make comment validation conditional in test --- test/test_suite_sqlalchemy.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/test/test_suite_sqlalchemy.py b/test/test_suite_sqlalchemy.py index afaa87e..213f02a 100644 --- a/test/test_suite_sqlalchemy.py +++ b/test/test_suite_sqlalchemy.py @@ -243,9 +243,10 @@ def test_get_multi_columns(self): len(actual[(None, "comment_test")]), len(expected[(None, "comment_test")]), ) - act = [x for x in actual[(None, "comment_test")] if x["name"] == "data"][0] - exp = [x for x in expected[(None, "comment_test")] if x["name"] == "data"][0] - eq_(act["comment"], exp["comment"]) + if config.db.dialect.supports_comments: + act = [x for x in actual[(None, "comment_test")] if x["name"] == "data"][0] + exp = [x for x in expected[(None, "comment_test")] if x["name"] == "data"][0] + eq_(act["comment"], exp["comment"]) @skip("cockroachdb") def test_get_multi_indexes(self):