This repository has been archived by the owner on Aug 13, 2019. It is now read-only.
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.
Add ChunksIterator method to Series interface. #665
base: master
Are you sure you want to change the base?
Add ChunksIterator method to Series interface. #665
Changes from 3 commits
64eeed9
4ed00e1
d097d3f
c407499
af8fb41
6f6dd39
78e9e36
f09fb89
220c3dd
b84c439
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 think it would be useful to also add a test for this.
It will help visualize what output is expected with a given input.
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.
Is there a test that covers this?
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 would rather prefer having an inefficient implementation as @brian-brazil said instead of an error here.
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 find this really hard to understand. I know it is used in the
verticalMergeSeriesIterator
as well, but will be really happy if we simplify this at some point.Not a blocker for this PR thought. I just thought I would mention it 😈
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 recursive approach is sometimes hard to follow indeed. Let's postpone cleaning this though, as we do this everywhere.
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 did run a debug session and the final struct is really bizarre, sort of deeply nested.
But yeah a conversation for another PR.