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

Added PrefetchIterator #214

Closed
wants to merge 1 commit into from
Closed

Added PrefetchIterator #214

wants to merge 1 commit into from

Conversation

t-novak
Copy link
Contributor

@t-novak t-novak commented Dec 11, 2017

Useful for construct like:

Dataset<T> data = Join.of(input, config)...;
ReduceByKey.of(data)
  .reduceBy((Stream<T> values, Collector<X> collector) -> {
    final PrefetchIterator<T> iterator = new PrefetchIterator<>(values);
    T first = iterator.prefetch();
    if (first != null) {
      if (first.selected())
        processSelected(iterator, collector);
      else
        processUnselected(iterator, collector);
    }
  });

Copy link
Contributor

@je-ik je-ik left a comment

Choose a reason for hiding this comment

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

Just a minor concern about naming the prefetch method. Otherwise 👍 and we can merge this then.

*
* @return Next value (or null on the end)
*/
public T prefetch() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can be name this peek? That is compliant with naming conventions of other methods performing this task.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm in doubt about peek().

It has different meaning in java 8 stream API and maybe someone would like to add PeekingStream later doing the same thing on stream.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say, that the naming of Stream#peek seems unfortunate, but OK. Can we come up with some other naming that would be clear to users? I have a strong feeling that the current naming is not self-explanatory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, more names:

  • LookupIterator::lookup()
  • PeekingIterator::peek()
  • PeepIterator::peep()
  • SurveyIterator::survey()
  • InspectIterator::inspect()
  • ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I still vote for peek, but maybe others might have some opinion on this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd also say that peek is most self explanatory method and people are used to it. Stream::peak method seems really unfortunate. I've just found out that Peeking iterator is already part of apache commons https://commons.apache.org/proper/commons-collections/apidocs/org/apache/commons/collections4/iterators/PeekingIterator.html and guava https://google.github.io/guava/releases/19.0/api/docs/com/google/common/collect/PeekingIterator.html

Shouldn't we use the one that guava provides?

Copy link
Contributor

Choose a reason for hiding this comment

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

@dmvk Cool, I wasn't aware of that. I think Iterators.peekingIterator(values.iterator()) does exactly this and we shouldn't reinvent the wheel. Also, as discussed over slack, motivation for this is probably missing #131
We should probably close this.

@t-novak t-novak closed this Dec 12, 2017
@dmvk dmvk deleted the tnovak/prefetch-iterator branch December 12, 2017 16:24
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