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

Iterate once to create two iterators in partition #2577

Merged
merged 15 commits into from
May 23, 2020
Prev Previous commit
Next Next commit
Avoid using io.vavr.collection.Stream
mincong-h committed May 5, 2020
commit 7c52962699131f07cdb03e5b3e455766a07c3a70
10 changes: 5 additions & 5 deletions src/main/java/io/vavr/collection/Iterator.java
Original file line number Diff line number Diff line change
@@ -1722,16 +1722,16 @@ default Tuple2<Iterator<T>, Iterator<T>> partition(Predicate<? super T> predicat
if (!hasNext()) {
return Tuple.of(empty(), empty());
} else {
Stream<T> first = Stream.empty();
Stream<T> second = Stream.empty();
final java.util.List<T> first = new ArrayList<>();
Copy link

Choose a reason for hiding this comment

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

I do not know if it is an issue here, but...
This solution is filtering eagerly and previous solution was filtering lazily.

If we would like to keep the same behaviour there is an option of creating a memoized predicate and then use it during filtering, e.g.

final Predicate<? super T> memoizedPredicate = Function1.of(predicate::test).memoized()::apply;
final Iterator<T> first = that.iterator().filter(memoizedPredicate);
final Iterator<T> second = that.iterator().filter(memoizedPredicate.negate());

Copy link

@kefasb kefasb May 8, 2020

Choose a reason for hiding this comment

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

I found that my solution with memoized predicate will not work in case when a mutable object mutates to the value that corresponds to another object to be filtered. Then another object is not evaluated but taken from cache with wrong result for it.
Example test:

final HashSet<MutableInteger> integers = HashSet.of(MutableInteger.of(1), MutableInteger.of(2));
final Tuple2<HashSet<MutableInteger>, HashSet<MutableInteger>> partition = integers.partition(mutableInteger -> mutableInteger.incrementAndCheckGreaterThan(2));
assertThat(partition._1).isEqualTo(HashSet.of(MutableInteger.of(3)));
assertThat(partition._2).isEqualTo(HashSet.of(MutableInteger.of(2)));

Where

    private static class MutableInteger {
        private int integer;
...
        boolean incrementAndCheckGreaterThan(int i) {
            return ++integer > i;
        }
...
    }

In this case the result of evaluation MutableInteger.of(1).incrementAndCheckGreaterThan(2) is put into cache with entry MutableInteger.of(2) -> false.
Then when the next object from set MutableInteger.of(2) is filtered and it is found in the cache and evaluated to false. While normally it would evaluate to MutableInteger.of(3) -> true.

But the question stays whether we want to keep the partitioning lazy or not.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a bit too technical for me... Maybe @danieldietrich can have an answer about it.

final java.util.List<T> second = new ArrayList<>();
for (T t : this) {
if (predicate.test(t)) {
first = first.append(t);
first.add(t);
} else {
second = second.append(t);
second.add(t);
}
}
return Tuple.of(first.iterator(), second.iterator());
return Tuple.of(ofAll(first), ofAll(second));
}
}