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

Add rolling stream method #581

Merged
merged 1 commit into from
Aug 9, 2019
Merged

Add rolling stream method #581

merged 1 commit into from
Aug 9, 2019

Conversation

benmccann
Copy link
Collaborator

@benmccann benmccann commented Aug 9, 2019

Thanks for contributing.

Description

This adds a rolling stream(n) method which allows us to do many different operations such as forEach, map, filter, anyMatch, allMatch, etc. on rolling groups of rows. rollWithRows(Consumer<Row[]> rows, int n) is the same as stream(n).forEach(consumer), so I've deprecated it since it's the same thing, but less powerful

@ryancerf depending on which PR is merged first (i.e. this one or #579) either you or I might need to rebase to put this method on Relation

Testing

Existing unit tests cover this functionality. They're effective too because they caught a bug in my initial implementation.

@benmccann
Copy link
Collaborator Author

What do you think about the name? Would rollingStream(n) be better than stream(n)?

@lwhite1
Copy link
Collaborator

lwhite1 commented Aug 9, 2019

This adds a rolling stream(n) method which allows us to do many different operations such as forEach, map, filter, anyMatch, allMatch, etc. on rolling groups of rows. rollWithRows(Consumer<Row[]> rows, int n) is the same as stream(n).forEach(consumer), so I've deprecated it since it's the same thing, but less powerful

There is so much awesome stuff getting done now. It's really exciting.

What do you think about the name? Would rollingStream(n) be better than stream(n)?

So long as you're rolling (i.e. moving forward one row at a time as in a rolling average), I think rollingStream is preferable. I often, also, find it useful to step forward by groups of n rows. We may want to support that option. Or is that built in here as well? If you can do either with this implementation I prefer stream().

Copy link
Collaborator

@lwhite1 lwhite1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than my one in-line comment and the replies to the PR, I think this is great. So much more powerful than what we have.

@ryancerf
Copy link
Collaborator

ryancerf commented Aug 9, 2019

This looks good to me, but I would like to discuss the possibility of supporting analytic functions. See #582. Analytic functions would provide similar rolling stream support.

@benmccann
Copy link
Collaborator Author

I renamed to rollingStream and added a steppingStream as well

@lwhite1 lwhite1 merged commit 134159d into master Aug 9, 2019
@benmccann benmccann deleted the stream-n branch August 19, 2019 02:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants