[SPARK-18200][GRAPHX] Support zero as an initial capacity in OpenHashSet #15741

dongjoon-hyun · 2016-11-02T22:56:57Z

What changes were proposed in this pull request?

SPARK-18200 reports Apache Spark 2.x raises java.lang.IllegalArgumentException: requirement failed: Invalid initial capacity while running triangleCount. The root cause is that VertexSet, a type alias of OpenHashSet, does not allow zero as a initial size. This PR loosens the restriction to allow zero.

How was this patch tested?

Pass the Jenkins test with a new test case in OpenHashSetSuite.

rxin · 2016-11-02T23:02:59Z

core/src/main/scala/org/apache/spark/util/collection/OpenHashSet.scala

@@ -42,10 +42,12 @@ import org.apache.spark.annotation.Private
 */
 @Private
 class OpenHashSet[@specialized(Long, Int) T: ClassTag](
-    initialCapacity: Int,
+    var initialCapacity: Int,


This looks really hacky and makes it a public variable. Why don't we just change the require to >= 0, and then change nextPowerOf2 to pick a value that's greater than 0?

Yep. I'll change like that.

SparkQA · 2016-11-03T01:20:37Z

Test build #68026 has finished for PR 15741 at commit d157ea1.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

rxin · 2016-11-03T01:23:46Z

core/src/main/scala/org/apache/spark/util/collection/OpenHashSet.scala

@@ -271,7 +271,7 @@ class OpenHashSet[@specialized(Long, Int) T: ClassTag](
  private def hashcode(h: Int): Int = Hashing.murmur3_32().hashInt(h).asInt()

  private def nextPowerOf2(n: Int): Int = {
-    val highBit = Integer.highestOneBit(n)
+    val highBit = Integer.highestOneBit(if (n == 0) 1 else n)


can you just do ...

if (n == 0) { 2 } else { // original code }

Sure.
BTW, for the test failure, I'll look at that soon.

SparkQA · 2016-11-03T01:37:06Z

Test build #68030 has finished for PR 15741 at commit d7bd9a9.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-11-03T03:53:06Z

Test build #68040 has finished for PR 15741 at commit b4dd73d.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

dongjoon-hyun · 2016-11-03T04:21:21Z

The only failure seems to be irrelevant.

[info] BasicSchedulerIntegrationSuite:
[info] - job with fetch failure *** FAILED *** (1 second, 889 milliseconds)
[info]   java.util.concurrent.TimeoutException: Futures timed out after [1 second]

dongjoon-hyun · 2016-11-03T04:21:36Z

Retest this please.

rxin · 2016-11-03T06:33:58Z

LGTM pending Jenkins.

SparkQA · 2016-11-03T06:43:21Z

Test build #68046 has finished for PR 15741 at commit b4dd73d.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

## What changes were proposed in this pull request? [SPARK-18200](https://issues.apache.org/jira/browse/SPARK-18200) reports Apache Spark 2.x raises `java.lang.IllegalArgumentException: requirement failed: Invalid initial capacity` while running `triangleCount`. The root cause is that `VertexSet`, a type alias of `OpenHashSet`, does not allow zero as a initial size. This PR loosens the restriction to allow zero. ## How was this patch tested? Pass the Jenkins test with a new test case in `OpenHashSetSuite`. Author: Dongjoon Hyun <[email protected]> Closes #15741 from dongjoon-hyun/SPARK-18200. (cherry picked from commit d24e736) Signed-off-by: Reynold Xin <[email protected]>

dongjoon-hyun · 2016-11-03T06:56:09Z

Thank you for review and merging, @rxin .

mridulm · 2016-11-03T07:50:54Z

core/src/main/scala/org/apache/spark/util/collection/OpenHashSet.scala

-    val highBit = Integer.highestOneBit(n)
-    if (highBit == n) n else highBit << 1
+    if (n == 0) {
+      2


This should have been 1 perhaps ?
nextPowerOf2(1) < nextPowerOf2(0) now ...

True or just make the condition <= 1. It won't really matter much but is a little inconsistent.

0 was designed to pretend to be 1.

nextPowerOf2(0) = nextPowerOf2(1) = 2

But, 1 could be better. Should I change this as a follow-up?

No I think the issue was that right now nextPowerOf2(1) is 1 and that seems inconsistent if 0 gives 2. Both should result in 2?

Oh, sorry. You're right. nextPowerOf2(1) is indeed 1. Then, both should return 1.

Then, should I fix this like the following?

- 2 + 1

Thank you for review, @mridulm and @srowen .
I'll create a PR as a follow-up.

…in OpenHashSet ## What changes were proposed in this pull request? This is a follow-up PR of apache#15741 in order to keep `nextPowerOf2` consistent. **Before** ``` nextPowerOf2(0) => 2 nextPowerOf2(1) => 1 nextPowerOf2(2) => 2 nextPowerOf2(3) => 4 nextPowerOf2(4) => 4 nextPowerOf2(5) => 8 ``` **After** ``` nextPowerOf2(0) => 1 nextPowerOf2(1) => 1 nextPowerOf2(2) => 2 nextPowerOf2(3) => 4 nextPowerOf2(4) => 4 nextPowerOf2(5) => 8 ``` ## How was this patch tested? N/A Author: Dongjoon Hyun <[email protected]> Closes apache#15754 from dongjoon-hyun/SPARK-18200-2.

…in OpenHashSet ## What changes were proposed in this pull request? This is a follow-up PR of #15741 in order to keep `nextPowerOf2` consistent. **Before** ``` nextPowerOf2(0) => 2 nextPowerOf2(1) => 1 nextPowerOf2(2) => 2 nextPowerOf2(3) => 4 nextPowerOf2(4) => 4 nextPowerOf2(5) => 8 ``` **After** ``` nextPowerOf2(0) => 1 nextPowerOf2(1) => 1 nextPowerOf2(2) => 2 nextPowerOf2(3) => 4 nextPowerOf2(4) => 4 nextPowerOf2(5) => 8 ``` ## How was this patch tested? N/A Author: Dongjoon Hyun <[email protected]> Closes #15754 from dongjoon-hyun/SPARK-18200-2. (cherry picked from commit 27602c3) Signed-off-by: Reynold Xin <[email protected]>

## What changes were proposed in this pull request? [SPARK-18200](https://issues.apache.org/jira/browse/SPARK-18200) reports Apache Spark 2.x raises `java.lang.IllegalArgumentException: requirement failed: Invalid initial capacity` while running `triangleCount`. The root cause is that `VertexSet`, a type alias of `OpenHashSet`, does not allow zero as a initial size. This PR loosens the restriction to allow zero. ## How was this patch tested? Pass the Jenkins test with a new test case in `OpenHashSetSuite`. Author: Dongjoon Hyun <[email protected]> Closes apache#15741 from dongjoon-hyun/SPARK-18200.

…in OpenHashSet ## What changes were proposed in this pull request? This is a follow-up PR of apache#15741 in order to keep `nextPowerOf2` consistent. **Before** ``` nextPowerOf2(0) => 2 nextPowerOf2(1) => 1 nextPowerOf2(2) => 2 nextPowerOf2(3) => 4 nextPowerOf2(4) => 4 nextPowerOf2(5) => 8 ``` **After** ``` nextPowerOf2(0) => 1 nextPowerOf2(1) => 1 nextPowerOf2(2) => 2 nextPowerOf2(3) => 4 nextPowerOf2(4) => 4 nextPowerOf2(5) => 8 ``` ## How was this patch tested? N/A Author: Dongjoon Hyun <[email protected]> Closes apache#15754 from dongjoon-hyun/SPARK-18200-2.

[SPARK-18200][GRAPHX] Support zero-size initial capacity in OpenHashSet

d157ea1

rxin reviewed Nov 2, 2016

View reviewed changes

dongjoon-hyun added 2 commits November 2, 2016 16:13

Address comments.

28879cb

Fix testcase name.

d7bd9a9

dongjoon-hyun changed the title ~~[SPARK-18200][GRAPHX] Support zero-size initial capacity in OpenHashSet~~ [SPARK-18200][GRAPHX] Support zero as an initial capacity in OpenHashSet Nov 2, 2016

rxin reviewed Nov 3, 2016

View reviewed changes

Remove testcase to expect the old behaviors.

b4dd73d

asfgit closed this in d24e736 Nov 3, 2016

mridulm reviewed Nov 3, 2016

View reviewed changes

dongjoon-hyun mentioned this pull request Nov 3, 2016

[SPARK-18200][GRAPHX][FOLLOW-UP] Support zero as an initial capacity in OpenHashSet #15754

Closed

dongjoon-hyun deleted the SPARK-18200 branch November 7, 2016 00:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-18200][GRAPHX] Support zero as an initial capacity in OpenHashSet #15741

[SPARK-18200][GRAPHX] Support zero as an initial capacity in OpenHashSet #15741

dongjoon-hyun commented Nov 2, 2016 •

edited

Loading

rxin Nov 2, 2016

dongjoon-hyun Nov 2, 2016

SparkQA commented Nov 3, 2016

rxin Nov 3, 2016

dongjoon-hyun Nov 3, 2016

SparkQA commented Nov 3, 2016

SparkQA commented Nov 3, 2016

dongjoon-hyun commented Nov 3, 2016

dongjoon-hyun commented Nov 3, 2016

rxin commented Nov 3, 2016

SparkQA commented Nov 3, 2016

dongjoon-hyun commented Nov 3, 2016

mridulm Nov 3, 2016

srowen Nov 3, 2016

dongjoon-hyun Nov 3, 2016

srowen Nov 3, 2016

dongjoon-hyun Nov 3, 2016

dongjoon-hyun Nov 3, 2016

dongjoon-hyun Nov 3, 2016

[SPARK-18200][GRAPHX] Support zero as an initial capacity in OpenHashSet #15741

[SPARK-18200][GRAPHX] Support zero as an initial capacity in OpenHashSet #15741

Conversation

dongjoon-hyun commented Nov 2, 2016 • edited Loading

What changes were proposed in this pull request?

How was this patch tested?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Nov 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Nov 3, 2016

SparkQA commented Nov 3, 2016

dongjoon-hyun commented Nov 3, 2016

dongjoon-hyun commented Nov 3, 2016

rxin commented Nov 3, 2016

SparkQA commented Nov 3, 2016

dongjoon-hyun commented Nov 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dongjoon-hyun commented Nov 2, 2016 •

edited

Loading