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: Check if replication key is timestamp before using start_date as default #1734

Draft
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

BuzzCutNorman
Copy link
Contributor

@BuzzCutNorman BuzzCutNorman commented May 24, 2023

This is an attempt to implement the proposed fix outlined in #1677

Closes #1677


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

@BuzzCutNorman BuzzCutNorman changed the title fix: Check if replication key is timestamp beofre using start_date as default fix: Check if replication key is timestamp before using start_date as default May 24, 2023
@BuzzCutNorman BuzzCutNorman marked this pull request as draft May 25, 2023 15:06
@BuzzCutNorman
Copy link
Contributor Author

@kgpayne thanks for pushing the target test fixes. I have noticed that I need to add in one of the fixes for #1676 locally before the sqlite tests run. The failures after the type error is corrected are in test_streams and have to do with Unix timestamp which were discussed last year in these links.

Should the SDK accommodate for differing start_date input formats #855
fix(taps): Use recent start_date as starting_replication_value: #759

I am not sure how to move forward with the Unix timestamp failures. The tests are there to keep SDK taps from breaking configurations that have been seen out in production. I am starting to wonder if I should resolve this in my tap-mssql and close this PR since it may break taps or pipelines that are in production. wdyt?

cc @edgarrmondragon

@kgpayne kgpayne added this to the v1.0 Release milestone Jun 27, 2023
@kgpayne
Copy link
Contributor

kgpayne commented Jun 27, 2023

@BuzzCutNorman I agree that, as a breaking change, we should keep this back until v1.0. Added it to the v1.0 Milestone 👍

@BuzzCutNorman
Copy link
Contributor Author

@kgpayne Thanks🙏 for the direction.

Copy link

codspeed-hq bot commented May 6, 2024

CodSpeed Performance Report

Merging #1734 will not alter performance

Comparing BuzzCutNorman:1677-fix-do-not-use-start-date-with-non-date-columns (6a6f850) with main (cb163bf)

Summary

✅ 6 untouched benchmarks

Copy link

codecov bot commented May 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.43%. Comparing base (cb163bf) to head (6a6f850).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1734      +/-   ##
==========================================
- Coverage   89.47%   89.43%   -0.05%     
==========================================
  Files          58       58              
  Lines        4799     4799              
  Branches      937      937              
==========================================
- Hits         4294     4292       -2     
- Misses        352      353       +1     
- Partials      153      154       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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: start_date from meltano.yml used even if replication-key column is not date-time
4 participants