Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Handle merging of SQL types when character column lengths are less than the max #1172

Merged
Show file tree
Hide file tree
Changes from 25 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
27f2b7d
Initial fix to handle character columns less than max
BuzzCutNorman Nov 11, 2022
589d9db
Merge branch 'main' into 1170-fix-merge-sql-type-to-handle-lengths-no…
BuzzCutNorman Nov 11, 2022
4975f6d
Merge branch 'main' into 1170-fix-merge-sql-type-to-handle-lengths-no…
BuzzCutNorman Nov 14, 2022
9e384c8
Merge branch 'main' into 1170-fix-merge-sql-type-to-handle-lengths-no…
BuzzCutNorman Nov 15, 2022
9523257
Merge branch 'main' into 1170-fix-merge-sql-type-to-handle-lengths-no…
BuzzCutNorman Nov 16, 2022
f982ec5
Merge branch 'main' into 1170-fix-merge-sql-type-to-handle-lengths-no…
BuzzCutNorman Nov 22, 2022
42adbf8
Merge branch 'main' into 1170-fix-merge-sql-type-to-handle-lengths-no…
BuzzCutNorman Nov 28, 2022
5842ade
fix for current_type.length max opt_len gt 0 type error
BuzzCutNorman Nov 28, 2022
7937f57
Merge branch 'main' into 1170-fix-merge-sql-type-to-handle-lengths-no…
BuzzCutNorman Dec 5, 2022
29ae501
Merge branch 'main' into 1170-fix-merge-sql-type-to-handle-lengths-no…
BuzzCutNorman Dec 7, 2022
97f6c16
Merge branch 'main' into 1170-fix-merge-sql-type-to-handle-lengths-no…
BuzzCutNorman Dec 8, 2022
7e13922
Merge branch 'main' into 1170-fix-merge-sql-type-to-handle-lengths-no…
edgarrmondragon Dec 8, 2022
cd2cf7b
Merge branch 'main' into 1170-fix-merge-sql-type-to-handle-lengths-no…
BuzzCutNorman Dec 12, 2022
a3bbc10
Merge branch 'main' into 1170-fix-merge-sql-type-to-handle-lengths-no…
BuzzCutNorman Dec 19, 2022
b968076
Apply suggestions from code review
BuzzCutNorman Dec 19, 2022
4e5b329
Merge branch 'main' into 1170-fix-merge-sql-type-to-handle-lengths-no…
BuzzCutNorman Jan 2, 2023
116aabd
Merge branch 'main' into 1170-fix-merge-sql-type-to-handle-lengths-no…
BuzzCutNorman Jan 6, 2023
c13fa90
Merge branch 'main' into 1170-fix-merge-sql-type-to-handle-lengths-no…
BuzzCutNorman Feb 6, 2023
80302f2
added some tests for merge_sql_types
BuzzCutNorman Feb 6, 2023
5fc5e41
refactor to resolve mypy error: TypeEngine[Any] has no attribute length
BuzzCutNorman Feb 6, 2023
118a1bd
trying to resolve mypy expression has type Optional[Any], variable ha…
BuzzCutNorman Feb 6, 2023
7ddfb7a
removed commneted out code
BuzzCutNorman Feb 6, 2023
50f1567
removed unused variable sql_type_len
BuzzCutNorman Feb 6, 2023
fd6ef9f
Merge branch 'main' of https://github.com/BuzzCutNorman/sdk into 1170…
BuzzCutNorman Mar 28, 2023
422cf93
Merge branch 'main' into 1170-fix-merge-sql-type-to-handle-lengths-no…
BuzzCutNorman Mar 28, 2023
e9a99db
utilizing connector fixture in test
BuzzCutNorman Mar 28, 2023
8dd1080
Merge branch 'main' into 1170-fix-merge-sql-type-to-handle-lengths-no…
edgarrmondragon Mar 28, 2023
ecda7b5
Update tests/core/test_connector_sql.py
edgarrmondragon Mar 28, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions poetry.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 6 additions & 6 deletions singer_sdk/connectors/sql.py
Original file line number Diff line number Diff line change
Expand Up @@ -834,11 +834,7 @@ def merge_sql_types(
# Gathering Type to match variables
# sent in _adapt_column_type
current_type = sql_types[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
cur_len: int = getattr(current_type, "length", 0)

# Convert the two types given into a sorted list
# containing the best conversion classes
Expand Down Expand Up @@ -866,7 +862,11 @@ 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 (cur_len and (opt_len >= cur_len))
):
return opt
# If best conversion class is equal to current type
# return the best conversion class
Expand Down
32 changes: 32 additions & 0 deletions tests/core/test_connector_sql.py
Original file line number Diff line number Diff line change
Expand Up @@ -197,3 +197,35 @@ def test_dialect_uses_engine(self, connector):
with mock.patch.object(attached_engine, "dialect") as _:
res = connector._dialect
assert res == attached_engine.dialect

def test_merge_sql_types_text_current_max(self):
edgarrmondragon marked this conversation as resolved.
Show resolved Hide resolved
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)
edgarrmondragon marked this conversation as resolved.
Show resolved Hide resolved
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