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

Conversation

BuzzCutNorman
Copy link
Contributor

@BuzzCutNorman BuzzCutNorman commented Nov 11, 2022

This is an attempt to fix merge_sql_type to handle instances when all passed character column lengths are less than max. SDK 0.13.1 needs PR #1168 present to reach this fix.

fixes #1170


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

@BuzzCutNorman
Copy link
Contributor Author

BuzzCutNorman commented Nov 11, 2022

@edgarrmondragon @aaronsteers I merged the latest main containing PR 1168 into this branch ran my tests locally. The code is working as planned and I am past the error:

ValueError: Unable to merge sql types: VARCHAR(64) COLLATE "SQL_Latin1_General_CP1_CI_AS", VARCHAR(64)

@BuzzCutNorman
Copy link
Contributor Author

BuzzCutNorman commented Nov 23, 2022

I just ran into an issue with this. If the current_type.length is None since it is set to max and the opt_len is anything greater than 0 you get a TypeError.

or (opt_len >= current_type.length) 
TypeError: '>=' not supported between instances of 'int' and 'NoneType' 

@BuzzCutNorman
Copy link
Contributor Author

I put in place a fix for the TypeError that I ran into.

cc: @edgarrmondragon @aaronsteers @cjohnhanson

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

codecov bot commented Feb 6, 2023

Codecov Report

Merging #1172 (ecda7b5) into main (61eecbf) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1172      +/-   ##
==========================================
+ Coverage   85.71%   85.73%   +0.01%     
==========================================
  Files          57       57              
  Lines        4691     4689       -2     
  Branches      802      801       -1     
==========================================
- Hits         4021     4020       -1     
  Misses        481      481              
+ Partials      189      188       -1     
Impacted Files Coverage Δ
singer_sdk/connectors/sql.py 85.45% <100.00%> (+0.25%) ⬆️

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

tests/core/test_connector_sql.py Outdated Show resolved Hide resolved
@edgarrmondragon edgarrmondragon changed the title fix: merge_sql_type to handle instances when all passed character column lengths are less than max fix: Handle merging of SQL types when character column lengths are less than max Mar 28, 2023
@edgarrmondragon edgarrmondragon changed the title fix: Handle merging of SQL types when character column lengths are less than max fix: Handle merging of SQL types when character column lengths are less than the max Mar 28, 2023
@edgarrmondragon edgarrmondragon self-requested a review March 28, 2023 22:11
@edgarrmondragon
Copy link
Collaborator

Thanks @BuzzCutNorman and sorry for the delay in reviews!

@edgarrmondragon edgarrmondragon merged commit 80efe93 into meltano:main Mar 28, 2023
@BuzzCutNorman
Copy link
Contributor Author

@edgarrmondragon, no worries. Thanks for all your great guidance.

@BuzzCutNorman BuzzCutNorman deleted the 1170-fix-merge-sql-type-to-handle-lengths-not-eq-max branch November 28, 2023 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: merge_sql_type() does not handle varchar or nvarchar lengths less than max
3 participants