-
Notifications
You must be signed in to change notification settings - Fork 437
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
Refactor - #2812 sequence separate performance #2818
Merged
nomisRev
merged 7 commits into
arrow-kt:main
from
Khepu:refactor/2812-sequence-separate-performance
Oct 17, 2022
Merged
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
0654b0c
Refactor: Use fold to separate Sequence of Either
Khepu c6ec40e
Refactor: Use fold to separate Sequence of Validated
Khepu 673bef7
Feature: Add tests for separating Either & Validated
Khepu bb66ed5
Chore: Evaluate sequences before assertion
Khepu 518e099
Merge branch 'main' into refactor/2812-sequence-separate-performance
nomisRev 0d1a1c4
Merge branch 'main' into refactor/2812-sequence-separate-performance
i-walker 3bb9d6c
Merge branch 'main' into refactor/2812-sequence-separate-performance
i-walker File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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.
IMO, the return type should be
Pair<List<A>, List<B>>
. Because we evaluate this sequence and return sequence is not lazyThere 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 do agree with that, is it something that I can change in this PR or would it be done along with other breaking changes?
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.
@hoc081098 that is a great point! We should probably post-pone that breaking change for Arrow 2.0. Shall we create an Arrow 2.0 ticket for the breaking change, and go ahead with these changes?
@Khepu the same improvement I commented on the
Iterable.kt
PR can probably be applied here too.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.
Just looked into how Sequence methods are implemented in the STD. It will be a bit different so as to avoid realizing the entire sequence just to separate it. Most of the implementations included an internal class that implements a custom iterator object. I should be able to have a PoC tomorrow though!