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(taps): Use recent start_date as starting_replication_value #759

Merged
merged 19 commits into from
Aug 23, 2022

Conversation

ericboucher
Copy link
Contributor

When both a start_date and a replication_start_date are available, I believe that we should take the most recent one.

@ericboucher ericboucher changed the title Use recent start_date as starting_replication_value feat: Use recent start_date as starting_replication_value Jun 27, 2022
@codecov
Copy link

codecov bot commented Jun 28, 2022

Codecov Report

Merging #759 (1ccab0a) into main (9dac11c) will increase coverage by 0.04%.
The diff coverage is 100.00%.

❗ Current head 1ccab0a differs from pull request most recent head a6cdfb8. Consider uploading reports for the commit a6cdfb8 to get more accurate results

@@            Coverage Diff             @@
##             main     #759      +/-   ##
==========================================
+ Coverage   80.33%   80.37%   +0.04%     
==========================================
  Files          34       34              
  Lines        3418     3425       +7     
  Branches      679      681       +2     
==========================================
+ Hits         2746     2753       +7     
  Misses        499      499              
  Partials      173      173              
Impacted Files Coverage Δ
singer_sdk/streams/core.py 81.21% <100.00%> (+0.33%) ⬆️

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

@edgarrmondragon edgarrmondragon self-assigned this Jun 28, 2022
singer_sdk/streams/core.py Outdated Show resolved Hide resolved
@ericboucher
Copy link
Contributor Author

@edgarrmondragon I added a check for is_timestamp_replication_key to make it a bit safer and explcit. wdyt?

Since we are in the subcase where a start_date is given as an input, I think this should be enough, but we could also add a try/except around the parsing.

As an alternative, we can indeed make a simple string comparison, although we are not sure what would happen when comparing a uuid and a date from "start_date".

I couldn't find if the format of start_date is actually enforced anywhere.

@edgarrmondragon
Copy link
Collaborator

@ericboucher ok, that makes sense and might be good for most use cases. Would you mind adding a test case, perhaps somewhere in

def test_stream_starting_timestamp(tap: SimpleTestTap, stream: SimpleTestStream):
?

@edgarrmondragon edgarrmondragon changed the title feat: Use recent start_date as starting_replication_value feat(taps): Use recent start_date as starting_replication_value Jul 15, 2022
@ericboucher
Copy link
Contributor Author

@edgarrmondragon done! Not sure why codecov is not passing though... Any clue? The UI is not very readable... 🤷

@edgarrmondragon
Copy link
Collaborator

edgarrmondragon commented Jul 15, 2022

@ericboucher it seems that codecov doesn't like that there isn't an else branch

Screen Shot 2022-07-15 at 18 23 22

I think I have a quick fix for that, and if that works I might push it to this branch 🙂


EDIT: Ok so I think that worked. @ericboucher let me know if the changes in d586071 make sense to you

singer_sdk/streams/core.py Outdated Show resolved Hide resolved
tests/core/test_streams.py Show resolved Hide resolved
@edgarrmondragon edgarrmondragon changed the title feat(taps): Use recent start_date as starting_replication_value fix(taps): Use recent start_date as starting_replication_value Jul 23, 2022
@ericboucher
Copy link
Contributor Author

@edgarrmondragon your changes make sense to me! Let me know if you any other changes from me :)

@aaronsteers aaronsteers dismissed their stale review August 22, 2022 20:49

feedback addressed and discussed

@edgarrmondragon edgarrmondragon force-pushed the start-date-replication-value branch 5 times, most recently from b39a41f to 1ccab0a Compare August 23, 2022 15:07
@edgarrmondragon edgarrmondragon force-pushed the start-date-replication-value branch from 1ccab0a to a6cdfb8 Compare August 23, 2022 15:08
@edgarrmondragon
Copy link
Collaborator

@ericboucher thanks for getting this started! It prompted a useful discussion on our side 😄

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.

3 participants