Skip to content
New issue

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

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

Already on GitHub? # to your account

fix: SQL Targets ignore collation when evaluating column data types #1385

Merged

Conversation

BuzzCutNorman
Copy link
Contributor

@BuzzCutNorman BuzzCutNorman commented Feb 2, 2023

This is an attempt to fix #1125


📚 Documentation preview 📚: https://meltano-sdk--1385.org.readthedocs.build/en/1385/

@codecov
Copy link

codecov bot commented Feb 3, 2023

Codecov Report

Merging #1385 (fd0548b) into main (9e2ae5c) will increase coverage by 0.00%.
The diff coverage is 85.71%.

@@           Coverage Diff           @@
##             main    #1385   +/-   ##
=======================================
  Coverage   85.19%   85.19%           
=======================================
  Files          54       54           
  Lines        4708     4722   +14     
  Branches      800      803    +3     
=======================================
+ Hits         4011     4023   +12     
- Misses        506      507    +1     
- Partials      191      192    +1     
Impacted Files Coverage Δ
singer_sdk/connectors/sql.py 78.27% <85.71%> (+0.41%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

singer_sdk/connectors/sql.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@edgarrmondragon edgarrmondragon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BuzzCutNorman remove_collation and update_collation both mutate the type object in-place, i.e. you'll notice the returned value points to the same object as the argument:

new_type = remove_collation(current_type)
assert new_type is current_type

So, do you think it makes to only return the collation from remove_collation and return nothing from update_collation. Similar to operations on a dictionary:

>>> d = {1: 2, 2: 3, 3: 4}
>>> d.pop(1)  # Return the removed value
2
>>> d.update(a=0)  # Return nothing, mutate in-place
>>> d
{2: 3, 3: 4, 'a': 0}

tests/core/test_connector_sql.py Outdated Show resolved Hide resolved
tests/core/test_connector_sql.py Outdated Show resolved Hide resolved
tests/core/test_connector_sql.py Outdated Show resolved Hide resolved
tests/core/test_connector_sql.py Outdated Show resolved Hide resolved
tests/core/test_connector_sql.py Outdated Show resolved Hide resolved
tests/core/test_connector_sql.py Outdated Show resolved Hide resolved
tests/core/test_connector_sql.py Outdated Show resolved Hide resolved
tests/core/test_connector_sql.py Outdated Show resolved Hide resolved
Co-authored-by: Edgar R. M. <edgarrm358@gmail.com>
@BuzzCutNorman
Copy link
Contributor Author

So, do you think it makes to only return the collation from remove_collation and return nothing from update_collation. Similar to operations on a dictionary:

@edgarrmondragon, Yes it does make sense and it looks much cleaner.

Copy link
Collaborator

@edgarrmondragon edgarrmondragon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @BuzzCutNorman, this is g2g!

@edgarrmondragon edgarrmondragon merged commit 984761e into meltano:main Feb 3, 2023
@BuzzCutNorman
Copy link
Contributor Author

Thank you @edgarrmondragon ! Couldn't have done it without your excellent guidance. I learned a lot today.

@BuzzCutNorman BuzzCutNorman deleted the 1125-sql-ignore-column-collation branch November 28, 2023 15:14
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: column collations should be ignored during SQL type comparisons (SQL Server)
2 participants