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: Ensure SQL streams are sorted when a replication key is set #1951

Conversation

edgarrmondragon
Copy link
Collaborator

@edgarrmondragon edgarrmondragon commented Sep 11, 2023

@edgarrmondragon edgarrmondragon changed the title Add counter-example snapshot fix: Ensure SQL streams are sorted when a replication key is set Sep 11, 2023
@edgarrmondragon edgarrmondragon force-pushed the 1950-bug-is_sorted-should-be-true-for-sqlstream-with-replication-keys branch from 74cd6b0 to 00798e0 Compare September 11, 2023 22:27
@edgarrmondragon
Copy link
Collaborator Author

edgarrmondragon commented Sep 12, 2023

SQLAlchemy 1.* tests are failing because inspected columns have a different order, so the SCHEMA messages, and thus the snapshots, are different.

EDIT: Not using snapshots anymore because of this 🤷‍♂️

@edgarrmondragon edgarrmondragon force-pushed the 1950-bug-is_sorted-should-be-true-for-sqlstream-with-replication-keys branch from 8ee00ec to 54e97b2 Compare September 12, 2023 00:13
@codecov
Copy link

codecov bot commented Sep 12, 2023

Codecov Report

Merging #1951 (9a67527) into main (10b61d2) will decrease coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1951      +/-   ##
==========================================
- Coverage   87.04%   87.01%   -0.04%     
==========================================
  Files          59       59              
  Lines        5109     5112       +3     
  Branches      827      827              
==========================================
+ Hits         4447     4448       +1     
- Misses        465      468       +3     
+ Partials      197      196       -1     
Files Changed Coverage Δ
singer_sdk/streams/sql.py 90.14% <100.00%> (+0.43%) ⬆️

... and 2 files with indirect coverage changes

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

@edgarrmondragon edgarrmondragon marked this pull request as ready for review September 12, 2023 00:24
Copy link
Contributor

@pnadolny13 pnadolny13 left a comment

Choose a reason for hiding this comment

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

@edgarrmondragon this looks good to me!

@edgarrmondragon edgarrmondragon merged commit 07198b0 into main Sep 13, 2023
@edgarrmondragon edgarrmondragon deleted the 1950-bug-is_sorted-should-be-true-for-sqlstream-with-replication-keys branch September 13, 2023 15:08
# 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: is_sorted should be true for SQLStream with replication keys
3 participants