From 27f2b7df3ced78b092eff5835b4e4b8e0b2f6d92 Mon Sep 17 00:00:00 2001 From: BuzzCutNorman Date: Fri, 11 Nov 2022 08:49:31 -0800 Subject: [PATCH 01/10] Initial fix to handle character columns less than max --- singer_sdk/connectors/sql.py | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/singer_sdk/connectors/sql.py b/singer_sdk/connectors/sql.py index 2bb102237..6597c036d 100644 --- a/singer_sdk/connectors/sql.py +++ b/singer_sdk/connectors/sql.py @@ -777,14 +777,22 @@ def merge_sql_types( (sqlalchemy.types.String, sqlalchemy.types.Unicode), ): # If length None or 0 then is varchar max ? - if (opt_len is None) or (opt_len == 0): + if ( + (opt_len is None) + or (opt_len == 0) + or (opt_len >= current_type.length) + ): return opt elif isinstance( generic_type, (sqlalchemy.types.String, sqlalchemy.types.Unicode), ): # If length None or 0 then is varchar max ? - if (opt_len is None) or (opt_len == 0): + if ( + (opt_len is None) + or (opt_len == 0) + or (opt_len >= current_type.length) + ): return opt # If best conversion class is equal to current type # return the best conversion class From 5842ade3de02af4865f3108f862f3de7238c647a Mon Sep 17 00:00:00 2001 From: BuzzCutNorman Date: Mon, 28 Nov 2022 11:15:46 -0800 Subject: [PATCH 02/10] fix for current_type.length max opt_len gt 0 type error --- singer_sdk/connectors/sql.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/singer_sdk/connectors/sql.py b/singer_sdk/connectors/sql.py index 53920e0ff..7be3abcb1 100644 --- a/singer_sdk/connectors/sql.py +++ b/singer_sdk/connectors/sql.py @@ -782,7 +782,7 @@ def merge_sql_types( if ( (opt_len is None) or (opt_len == 0) - or (opt_len >= current_type.length) + or (opt_len >= (current_type.length or (opt_len + 1))) ): return opt elif isinstance( @@ -793,7 +793,7 @@ def merge_sql_types( if ( (opt_len is None) or (opt_len == 0) - or (opt_len >= current_type.length) + or (opt_len >= (current_type.length or (opt_len + 1))) ): return opt # If best conversion class is equal to current type From b968076d0a989e9685a40064d12466647020e07e Mon Sep 17 00:00:00 2001 From: Dan Norman Date: Mon, 19 Dec 2022 11:36:03 -0700 Subject: [PATCH 03/10] Apply suggestions from code review Co-authored-by: Edgar R. M. --- singer_sdk/connectors/sql.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/singer_sdk/connectors/sql.py b/singer_sdk/connectors/sql.py index 7be3abcb1..03b15b4ad 100644 --- a/singer_sdk/connectors/sql.py +++ b/singer_sdk/connectors/sql.py @@ -782,7 +782,7 @@ def merge_sql_types( if ( (opt_len is None) or (opt_len == 0) - or (opt_len >= (current_type.length or (opt_len + 1))) + or (current_type.length and (opt_len >= current_type.length)) ): return opt elif isinstance( @@ -793,7 +793,7 @@ def merge_sql_types( if ( (opt_len is None) or (opt_len == 0) - or (opt_len >= (current_type.length or (opt_len + 1))) + or (current_type.length and (opt_len >= current_type.length)) ): return opt # If best conversion class is equal to current type From 80302f2ed9de14ad66282639ebc938d97983cd19 Mon Sep 17 00:00:00 2001 From: BuzzCutNorman Date: Mon, 6 Feb 2023 11:13:49 -0800 Subject: [PATCH 04/10] added some tests for merge_sql_types --- tests/core/test_connector_sql.py | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/tests/core/test_connector_sql.py b/tests/core/test_connector_sql.py index 8ad7bf9a1..ed11d8ee3 100644 --- a/tests/core/test_connector_sql.py +++ b/tests/core/test_connector_sql.py @@ -130,3 +130,35 @@ def test_update_collation_non_text_type(self): assert not hasattr(compatible_type, "collation") # Check that we get the same type we put in assert str(compatible_type) == "INTEGER" + + def test_merge_sql_types_text_current_max(self): + cls = SQLConnector() + current_type = sqlalchemy.types.VARCHAR(length=None) + sql_type = sqlalchemy.types.VARCHAR(length=255) + compatible_sql_type = cls.merge_sql_types([current_type, sql_type]) + # Check that the current VARCHAR(MAX) type is kept + assert compatible_sql_type is current_type + + def test_merge_sql_types_text_current_greater_than(self): + cls = SQLConnector() + current_type = sqlalchemy.types.VARCHAR(length=255) + sql_type = sqlalchemy.types.VARCHAR(length=64) + compatible_sql_type = cls.merge_sql_types([current_type, sql_type]) + # Check the current greater VARCHAR(255) is kept + assert compatible_sql_type is current_type + + def test_merge_sql_types_text_proposed_max(self): + cls = SQLConnector() + current_type = sqlalchemy.types.VARCHAR(length=64) + sql_type = sqlalchemy.types.VARCHAR(length=None) + compatible_sql_type = cls.merge_sql_types([current_type, sql_type]) + # Check the current VARCHAR(64) is chosen over default varcahr(max) + assert compatible_sql_type is current_type + + def test_merge_sql_types_text_current_less_than(self): + cls = SQLConnector() + current_type = sqlalchemy.types.VARCHAR(length=64) + sql_type = sqlalchemy.types.VARCHAR(length=255) + compatible_sql_type = cls.merge_sql_types([current_type, sql_type]) + # Check that VARCHAR(255) is chosen over the lesser current VARCHAR(64) + assert compatible_sql_type is sql_type From 5fc5e41e9b292a05ec44aea61ba11b6867b57cf6 Mon Sep 17 00:00:00 2001 From: BuzzCutNorman Date: Mon, 6 Feb 2023 12:03:31 -0800 Subject: [PATCH 05/10] refactor to resolve mypy error: TypeEngine[Any] has no attribute length --- singer_sdk/connectors/sql.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/singer_sdk/connectors/sql.py b/singer_sdk/connectors/sql.py index 86e0b53ce..75a3ca1d5 100644 --- a/singer_sdk/connectors/sql.py +++ b/singer_sdk/connectors/sql.py @@ -748,6 +748,7 @@ def merge_sql_types( # Gathering Type to match variables # sent in _adapt_column_type current_type = sql_types[0] + cur_len: int = getattr(current_type, "length", None) # sql_type = sql_types[1] # Getting the length of each type @@ -782,7 +783,7 @@ def merge_sql_types( if ( (opt_len is None) or (opt_len == 0) - or (current_type.length and (opt_len >= current_type.length)) + or (cur_len and (opt_len >= cur_len)) ): return opt elif isinstance( @@ -793,7 +794,7 @@ def merge_sql_types( if ( (opt_len is None) or (opt_len == 0) - or (current_type.length and (opt_len >= current_type.length)) + or (cur_len and (opt_len >= cur_len)) ): return opt # If best conversion class is equal to current type From 118a1bdd5e68c04ebf1f05d19ac3517d6aa866ee Mon Sep 17 00:00:00 2001 From: BuzzCutNorman Date: Mon, 6 Feb 2023 14:21:12 -0800 Subject: [PATCH 06/10] trying to resolve mypy expression has type Optional[Any], variable has type int --- singer_sdk/connectors/sql.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/singer_sdk/connectors/sql.py b/singer_sdk/connectors/sql.py index 75a3ca1d5..9e9e60b4c 100644 --- a/singer_sdk/connectors/sql.py +++ b/singer_sdk/connectors/sql.py @@ -748,7 +748,7 @@ def merge_sql_types( # Gathering Type to match variables # sent in _adapt_column_type current_type = sql_types[0] - cur_len: int = getattr(current_type, "length", None) + cur_len: int = getattr(current_type, "length", 0) # sql_type = sql_types[1] # Getting the length of each type From 7ddfb7a3bbd40b5c43abddedaf7b1ac782fcbd5d Mon Sep 17 00:00:00 2001 From: BuzzCutNorman Date: Mon, 6 Feb 2023 14:31:05 -0800 Subject: [PATCH 07/10] removed commneted out code --- singer_sdk/connectors/sql.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/singer_sdk/connectors/sql.py b/singer_sdk/connectors/sql.py index 9e9e60b4c..0afbdaade 100644 --- a/singer_sdk/connectors/sql.py +++ b/singer_sdk/connectors/sql.py @@ -749,10 +749,8 @@ def merge_sql_types( # sent in _adapt_column_type current_type = sql_types[0] cur_len: int = getattr(current_type, "length", 0) - # sql_type = sql_types[1] # Getting the length of each type - # current_type_len: int = getattr(sql_types[0], "length", 0) sql_type_len: int = getattr(sql_types[1], "length", 0) if sql_type_len is None: sql_type_len = 0 From 50f156707a2b4dd3aec13dcd6650bde7bd79795b Mon Sep 17 00:00:00 2001 From: BuzzCutNorman Date: Mon, 6 Feb 2023 14:39:51 -0800 Subject: [PATCH 08/10] removed unused variable sql_type_len --- singer_sdk/connectors/sql.py | 5 ----- 1 file changed, 5 deletions(-) diff --git a/singer_sdk/connectors/sql.py b/singer_sdk/connectors/sql.py index 0afbdaade..ad3fc3c6f 100644 --- a/singer_sdk/connectors/sql.py +++ b/singer_sdk/connectors/sql.py @@ -750,11 +750,6 @@ def merge_sql_types( current_type = sql_types[0] cur_len: int = getattr(current_type, "length", 0) - # Getting the length of each type - sql_type_len: int = getattr(sql_types[1], "length", 0) - if sql_type_len is None: - sql_type_len = 0 - # Convert the two types given into a sorted list # containing the best conversion classes sql_types = self._sort_types(sql_types) From e9a99db243571edc6c0aa751b1ff18b69f77efbd Mon Sep 17 00:00:00 2001 From: BuzzCutNorman Date: Tue, 28 Mar 2023 14:10:38 -0700 Subject: [PATCH 09/10] utilizing connector fixture in test --- tests/core/test_connector_sql.py | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/tests/core/test_connector_sql.py b/tests/core/test_connector_sql.py index 51e20783d..8c2923b3a 100644 --- a/tests/core/test_connector_sql.py +++ b/tests/core/test_connector_sql.py @@ -198,34 +198,30 @@ def test_dialect_uses_engine(self, connector): res = connector._dialect assert res == attached_engine.dialect - def test_merge_sql_types_text_current_max(self): - cls = SQLConnector() + def test_merge_sql_types_text_current_max(self, connector): current_type = sqlalchemy.types.VARCHAR(length=None) sql_type = sqlalchemy.types.VARCHAR(length=255) - compatible_sql_type = cls.merge_sql_types([current_type, sql_type]) + compatible_sql_type = connector.merge_sql_types([current_type, sql_type]) # Check that the current VARCHAR(MAX) type is kept assert compatible_sql_type is current_type - def test_merge_sql_types_text_current_greater_than(self): - cls = SQLConnector() + def test_merge_sql_types_text_current_greater_than(self, connector): current_type = sqlalchemy.types.VARCHAR(length=255) sql_type = sqlalchemy.types.VARCHAR(length=64) - compatible_sql_type = cls.merge_sql_types([current_type, sql_type]) + compatible_sql_type = connector.merge_sql_types([current_type, sql_type]) # Check the current greater VARCHAR(255) is kept assert compatible_sql_type is current_type - def test_merge_sql_types_text_proposed_max(self): - cls = SQLConnector() + def test_merge_sql_types_text_proposed_max(self, connector): current_type = sqlalchemy.types.VARCHAR(length=64) sql_type = sqlalchemy.types.VARCHAR(length=None) - compatible_sql_type = cls.merge_sql_types([current_type, sql_type]) + compatible_sql_type = connector.merge_sql_types([current_type, sql_type]) # Check the current VARCHAR(64) is chosen over default varcahr(max) assert compatible_sql_type is current_type - def test_merge_sql_types_text_current_less_than(self): - cls = SQLConnector() + def test_merge_sql_types_text_current_less_than(self, connector): current_type = sqlalchemy.types.VARCHAR(length=64) sql_type = sqlalchemy.types.VARCHAR(length=255) - compatible_sql_type = cls.merge_sql_types([current_type, sql_type]) + compatible_sql_type = connector.merge_sql_types([current_type, sql_type]) # Check that VARCHAR(255) is chosen over the lesser current VARCHAR(64) assert compatible_sql_type is sql_type From ecda7b59edc6a61fd8dc2423b09ea0ed0d19a6a7 Mon Sep 17 00:00:00 2001 From: "Edgar R. M" Date: Tue, 28 Mar 2023 16:10:11 -0600 Subject: [PATCH 10/10] Update tests/core/test_connector_sql.py --- tests/core/test_connector_sql.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/core/test_connector_sql.py b/tests/core/test_connector_sql.py index 4626202b6..dc16264e1 100644 --- a/tests/core/test_connector_sql.py +++ b/tests/core/test_connector_sql.py @@ -216,7 +216,7 @@ def test_merge_sql_types_text_proposed_max(self, connector): current_type = sqlalchemy.types.VARCHAR(length=64) sql_type = sqlalchemy.types.VARCHAR(length=None) compatible_sql_type = connector.merge_sql_types([current_type, sql_type]) - # Check the current VARCHAR(64) is chosen over default varcahr(max) + # Check the current VARCHAR(64) is chosen over default VARCHAR(max) assert compatible_sql_type is current_type def test_merge_sql_types_text_current_less_than(self, connector):