Skip to content

[SPARK-12376][TESTS] Spark Streaming Java8APISuite fails in assertOrderInvariantEquals method #10336

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.

Sign up for GitHub

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

Closed
evanyc15 wants to merge 3 commits into from
Closed

[SPARK-12376][TESTS] Spark Streaming Java8APISuite fails in assertOrderInvariantEquals method #10336

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
28ff404
Changes to Spark Streaming Java8APISuite.java
Dec 16, 2015
23ace37
Changed ArrayList to List type
Dec 16, 2015
8addf75
Removed foreach argument type
Dec 16, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions extras/java8-tests/src/test/java/org/apache/spark/streaming/Java8APISuite.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import com.google.common.base.Optional;
import com.google.common.collect.Lists;
import com.google.common.collect.Sets;
import com.google.common.collect.Ordering;

@srowen srowen Dec 16, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think this is used.

import org.junit.Assert;
import org.junit.Test;

Expand Down Expand Up @@ -440,8 +441,13 @@ public void testPairFlatMap() {
public static <T extends Comparable<T>> void assertOrderInvariantEquals(
List<List<T>> expected, List<List<T>> actual) {
expected.forEach((List<T> list) -> Collections.sort(list));
actual.forEach((List<T> list) -> Collections.sort(list));
Assert.assertEquals(expected, actual);
ArrayList<ArrayList<T>> sortedActual = new ArrayList<ArrayList<T>>();

@srowen srowen Dec 16, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this can be List<List<T>> sortedActual = new ArrayList<>(); right? And similarly below.

actual.forEach((List<T> list) -> {

@srowen srowen Dec 16, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can this be cleverer, like

List<List<T>> actualSorted = actual.stream().map(unsorted -> {
  List<T> sorted = new ArrayList<>(unsorted);
  Collections.sort(sorted);
  return sorted;
});

@evanyc15 evanyc15 Dec 16, 2015

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Hey srowen,

I feel like this can be more confusing to follow. Does your code alternative have any increases in performance?

Thank you

@srowen srowen Dec 16, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Really? it's a little less code and matches the functional style of other (Scala) code. Performance is not an issue here. However I forgot the collect(). How about:

List<List<T>> actualSorted = actual.stream().
  map(l -> new ArrayList<>(l)).map(Collections::sort).
  collect(Collectors.toList());

EDIT: nah that doesn't quite work since sort doesn't return the list. No big deal, whatever of this seems clearest.

@srowen srowen Dec 16, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The arg does not need a type.

ArrayList<T> sortedList = new ArrayList<T>(list);
Collections.sort(sortedList);
sortedActual.add(sortedList);
});
Assert.assertEquals(expected, sortedActual);
}

@Test
Expand Down