Repository: meltano/sdk (python)

Issue:
bug: column collations should be ignored during SQL type comparisons (SQL Server)
### Singer SDK Version

0.11.1

### Python Version

3.10

### Bug scope

Targets (data type handling, batching, SQL object generation, etc.)

### Operating System

Mac

### Description

SQL Server target: Initial load of a table with a varchar column works nicely, but on second write, the loader fails with a ValueError, after attempting to change column types. In the example, a PG source table with a VARCHAR(64) column:

`ValueError: Unable to merge sql types: VARCHAR(64) COLLATE "SQL_Latin1_General_CP1_CI_AS", VARCHAR(64)`

My interpretation is that the target initially ran a create table statement with a VARCHAR(64) column, SQL Server added the collation as a detail, but when inserting again the target interprets varchar with the added collation as a different column type than VARCHAR(64) and tries to alter it, but fails.

An ugly fix is to explicitly declare the collation on table creation: https://github.com/radbrt/target-mssql/blob/37544ad6306095b66bef9f01d35211f486b9bf77/target_mssql/sinks.py#L262


### Code

_No response_

Pull request:
fix: SQL Targets ignore collation when evaluating column data types
This is an attempt to fix #1125

<!-- readthedocs-preview meltano-sdk start -->
----
:books: Documentation preview :books:: https://meltano-sdk--1385.org.readthedocs.build/en/1385/

<!-- readthedocs-preview meltano-sdk end -->

Patch:
diff --git a/poetry.lock b/poetry.lock
index a1a59358..f8fdb18b 100644
--- a/poetry.lock
+++ b/poetry.lock
@@ -1012,7 +1012,6 @@ category = "main"
 optional = true
 python-versions = "*"
 files = [
-    {file = "livereload-2.6.3-py2.py3-none-any.whl", hash = "sha256:ad4ac6f53b2d62bb6ce1a5e6e96f1f00976a32348afedcb4b6d68df2a1d346e4"},
     {file = "livereload-2.6.3.tar.gz", hash = "sha256:776f2f865e59fde56490a56bcc6773b6917366bce0c267c60ee8aaf1a0959869"},
 ]
 
@@ -1597,7 +1596,7 @@ files = [
 cffi = ">=1.4.1"
 
 [package.extras]
-docs = ["sphinx (>=1.6.5)", "sphinx-rtd-theme"]
+docs = ["sphinx (>=1.6.5)", "sphinx_rtd_theme"]
 tests = ["hypothesis (>=3.27.0)", "pytest (>=3.2.1,!=3.3.0)"]
 
 [[package]]
@@ -2243,7 +2242,7 @@ importlib-metadata = {version = "*", markers = "python_version < \"3.8\""}
 
 [package.extras]
 aiomysql = ["aiomysql", "greenlet (!=0.4.17)"]
-aiosqlite = ["aiosqlite", "greenlet (!=0.4.17)", "typing-extensions (!=3.10.0.1)"]
+aiosqlite = ["aiosqlite", "greenlet (!=0.4.17)", "typing_extensions (!=3.10.0.1)"]
 asyncio = ["greenlet (!=0.4.17)"]
 asyncmy = ["asyncmy (>=0.2.3,!=0.2.4)", "greenlet (!=0.4.17)"]
 mariadb-connector = ["mariadb (>=1.0.1,!=1.1.2)"]
@@ -2253,14 +2252,14 @@ mssql-pyodbc = ["pyodbc"]
 mypy = ["mypy (>=0.910)", "sqlalchemy2-stubs"]
 mysql = ["mysqlclient (>=1.4.0)", "mysqlclient (>=1.4.0,<2)"]
 mysql-connector = ["mysql-connector-python"]
-oracle = ["cx-oracle (>=7)", "cx-oracle (>=7,<8)"]
+oracle = ["cx_oracle (>=7)", "cx_oracle (>=7,<8)"]
 postgresql = ["psycopg2 (>=2.7)"]
 postgresql-asyncpg = ["asyncpg", "greenlet (!=0.4.17)"]
 postgresql-pg8000 = ["pg8000 (>=1.16.6,!=1.29.0)"]
 postgresql-psycopg2binary = ["psycopg2-binary"]
 postgresql-psycopg2cffi = ["psycopg2cffi"]
 pymysql = ["pymysql", "pymysql (<1)"]
-sqlcipher = ["sqlcipher3-binary"]
+sqlcipher = ["sqlcipher3_binary"]
 
 [[package]]
 name = "sqlalchemy2-stubs"
diff --git a/singer_sdk/connectors/sql.py b/singer_sdk/connectors/sql.py
index 5aae327a..d3ec8126 100644
--- a/singer_sdk/connectors/sql.py
+++ b/singer_sdk/connectors/sql.py
@@ -943,6 +943,37 @@ class SQLConnector:
             },
         )
 
+    @staticmethod
+    def remove_collation(
+        column_type: sqlalchemy.types.TypeEngine,
+    ) -> str | None:
+        """Removes collation for the given column TypeEngine instance.
+
+        Args:
+            column_type: Column SQLAlchemy type.
+
+        Returns:
+             The removed collation as a string.
+        """
+        if hasattr(column_type, "collation") and column_type.collation:
+            column_type_collation: str = column_type.collation
+            setattr(column_type, "collation", None)
+            return column_type_collation
+        return None
+
+    @staticmethod
+    def update_collation(
+        column_type: sqlalchemy.types.TypeEngine, collation: str | None
+    ) -> None:
+        """Sets column collation if column type has a collation attribute.
+
+        Args:
+            column_type: Column SQLAlchemy type.
+            collation: The colation
+        """
+        if hasattr(column_type, "collation") and collation:
+            setattr(column_type, "collation", collation)
+
     def _adapt_column_type(
         self,
         full_table_name: str,
@@ -963,6 +994,9 @@ class SQLConnector:
             full_table_name, column_name
         )
 
+        # remove collation if present and save it
+        current_type_collation = self.remove_collation(current_type)
+
         # Check if the existing column type and the sql type are the same
         if str(sql_type) == str(current_type):
             # The current column and sql type are the same
@@ -977,6 +1011,10 @@ class SQLConnector:
             # Nothing to do
             return
 
+        # Put the collation level back before altering the column
+        if current_type_collation:
+            self.update_collation(compatible_sql_type, current_type_collation)
+
         if not self.allow_column_alter:
             raise NotImplementedError(
                 "Altering columns is not supported. "


Test patch:
diff --git a/tests/core/test_connector_sql.py b/tests/core/test_connector_sql.py
index 518b17bd..8ad7bf9a 100644
--- a/tests/core/test_connector_sql.py
+++ b/tests/core/test_connector_sql.py
@@ -91,3 +91,42 @@ class TestConnectorSQL:
             )
         )
         assert statement == rendered_statement
+
+    def test_remove_collation_text_type(self):
+        remove_collation = SQLConnector.remove_collation
+        test_collation = "SQL_Latin1_General_CP1_CI_AS"
+        current_type = sqlalchemy.types.Text(collation=test_collation)
+        current_type_collation = remove_collation(current_type)
+        # Check collation was set to None by the function
+        assert current_type.collation is None
+        # Check that we get the same collation we put in back out
+        assert current_type_collation == test_collation
+
+    def test_remove_collation_non_text_type(self):
+        remove_collation = SQLConnector.remove_collation
+        current_type = sqlalchemy.types.Integer()
+        current_type_collation = remove_collation(current_type)
+        # Check there is not a collation attribute
+        assert not hasattr(current_type, "collation")
+        # Check that we get the same type we put in
+        assert str(current_type) == "INTEGER"
+        # Check that this variable is missing
+        assert current_type_collation is None
+
+    def test_update_collation_text_type(self):
+        update_collation = SQLConnector.update_collation
+        test_collation = "SQL_Latin1_General_CP1_CI_AS"
+        compatible_type = sqlalchemy.types.Text(collation=None)
+        update_collation(compatible_type, test_collation)
+        # Check collation was set to the value we put in
+        assert compatible_type.collation == test_collation
+
+    def test_update_collation_non_text_type(self):
+        update_collation = SQLConnector.update_collation
+        test_collation = "SQL_Latin1_General_CP1_CI_AS"
+        compatible_type = sqlalchemy.types.Integer()
+        update_collation(compatible_type, test_collation)
+        # Check there is not a collation attribute
+        assert not hasattr(compatible_type, "collation")
+        # Check that we get the same type we put in
+        assert str(compatible_type) == "INTEGER"