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
14 changes: 10 additions & 4 deletions src/main/java/io/vavr/collection/Iterator.java
Original file line number Diff line number Diff line change
Expand Up @@ -1722,10 +1722,16 @@ default Tuple2<Iterator<T>, Iterator<T>> partition(Predicate<? super T> predicat
if (!hasNext()) {
return Tuple.of(empty(), empty());
} else {
final Stream<T> that = Stream.ofAll(this);
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.

There is also partition method implemented by Stream which works wrongly too.

I do not know another places. Maybe @danieldietrich could point them out.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was not aware of the problem of Stream#partition. Thanks for pointing it out! 🙇 I checked all data-structures in VAVR, apart from Stream, all other implementations work correctly. About the Stream itself, I'm afraid that I don't know how to fix it and I'm not sure if it's really a bug. In Scala 2.13.2, the Stream implementation uses the predicates twice (link) as below:

override def partition(p: A => Boolean): (Stream[A], Stream[A]) = (filter(p(_)), filterNot(p(_)))

If I modify your example in issue #2559 by changing Set into Stream, you will see result in Scala as follow:

  "Stream" should "partition correctly" in {
    val fruitsToEat = Stream("apple", "banana")
    val partition = fruitsToEat.partition(name => biteAndCheck(name))

    partition._1 shouldEqual Stream()
    partition._2 shouldEqual Stream()  // not Stream("apple", "banana")

    fruitsBeingEaten.get("apple").get.name shouldEqual "apple"
    fruitsBeingEaten.get("apple").get.bites shouldEqual 2  // not 1
    fruitsBeingEaten.get("banana").get.name shouldEqual "banana"
    fruitsBeingEaten.get("banana").get.bites shouldEqual 2  // not 1
  }

So I would say the VAVR is aligned with Scala on Stream's behaviors. Also, my current approach (ArrayList) does not fit the Stream requirement, because Stream is lazy sequence of elements which may be infinitely long. So I don't know how to use only one predicate to achieve that... So my suggestion is let's keep stream as it is and avoid modifying it.

Copy link
Member

Choose a reason for hiding this comment

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

Stream is lazy sequence of elements which may be infinitely long

Oh sorry guys I completely forgot about it :(

final Iterator<T> first = that.iterator().filter(predicate);
final Iterator<T> second = that.iterator().filter(predicate.negate());
return Tuple.of(first, second);
Stream<T> first = Stream.empty();
Stream<T> second = Stream.empty();
for (T t : this) {
if (predicate.test(t)) {
first = first.append(t);
Copy link
Member

Choose a reason for hiding this comment

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

As far as I remember, Stream.append is an O(n) operation, so the whole op will be O(n^2).
I believe you should use two java's ArrayList objects to partition.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for your pointer @ruslansennov . This should be fixed now.

} else {
second = second.append(t);
}
}
return Tuple.of(first.iterator(), second.iterator());
}
}

Expand Down
86 changes: 86 additions & 0 deletions src/test/java/io/vavr/issues/Issue2559Test.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
package io.vavr.issues;

import io.vavr.Tuple2;
import io.vavr.collection.HashSet;
import io.vavr.collection.Set;
import org.junit.Before;
import org.junit.Test;

import java.util.Objects;

import static org.assertj.core.api.Assertions.assertThat;

/**
* partition method seems to not work correctly, comparing to scala
* https://github.com/vavr-io/vavr/issues/2559
*/
public class Issue2559Test {

private java.util.Map<String, Eat> fruitsBeingEaten = new java.util.HashMap<>();

@Before
public void setUp() {
fruitsBeingEaten = new java.util.HashMap<>();
}

@Test
public void partitionShouldBeUnique() {
final Set<String> fruitsToEat = HashSet.of("apple", "banana");
final Tuple2<? extends Set<String>, ? extends Set<String>> partition = fruitsToEat.partition(this::biteAndCheck);
assertThat(partition._1).isEmpty();
assertThat(partition._2).isEqualTo(HashSet.of("apple", "banana"));
assertThat(fruitsBeingEaten)
.hasSize(2)
.containsEntry("apple", new Eat(1, "apple"))
.containsEntry("banana", new Eat(1, "banana"));
}

private boolean biteAndCheck(String name) {
final Eat eat = fruitsBeingEaten.getOrDefault(name, Eat.prepare(name)).bite();
fruitsBeingEaten.put(name, eat);
return eat.isEaten();
}

private static class Eat {
final int bites;
final String name;

public static Eat prepare(String name) {
return new Eat(0, name);
}

private Eat(int bites, String name) {
this.bites = bites;
this.name = name;
}

public Eat bite() {
return new Eat(bites + 1, name);
}

public boolean isEaten() {
return bites >= 2;
}

@Override
public boolean equals(Object o) {
if (this == o) return true;
if (!(o instanceof Eat)) return false;
Eat eat = (Eat) o;
return bites == eat.bites && Objects.equals(name, eat.name);
}

@Override
public int hashCode() {
return Objects.hash(bites, name);
}

@Override
public String toString() {
return "Eat{" +
"bites=" + bites +
", name='" + name + '\'' +
'}';
}
}
}