Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: add incremental lag for datetime, int, and float cursors #1957
feat: add incremental lag for datetime, int, and float cursors #1957
Changes from 3 commits
b29d75e
17dc69a
a06d552
ccc68ab
0f44fe4
36bbbaf
f5c860e
6a63cf6
56e492d
80926b7
d38ab87
3cdf773
45ad90f
afba568
a1e3263
6a3c4d3
e883e94
717acbf
00b01e8
401587d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have similar comments as to the test below:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implemented your comments except for number 2:
I have used
@pytest.mark.parametrize("lag", [-1, 10])
to execute the code with different lags instead of using apply_hints.Also facing the deduplication bug - more info above in the other comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the concept of test is good. but IMO you should create just one resource with merge write disposition that you call with the lag you want from the very beginning. this is how people will use it IMO.
emulate returning "updated_events" on the second call to it ie. via some kind of nonlocal flag that tells to add those events on the second time.
the results of the test look good but please test two additional things
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, there is a deduplication bug - I have captured it in the tests but surprisingly setting deduplication_disabled to True doesn't solve issue. Any tips on how to solve it?
I have refactored the code and tests with your suggestions.
Question, what do you mean with the following point?
In the currently implementation lag only applies to last_value but I see that initially last_value is set to initial_value on
get_state
.Do I need to ignore the _apply_lag exec when last_value == initial_value?