[SPARK-25542][Core][Test] Move flaky test in OpenHashMapSuite to OpenHashSetSuite and make it against OpenHashSet #22569

viirya · 2018-09-27T10:09:53Z

What changes were proposed in this pull request?

The specified test in OpenHashMapSuite to test large items is somehow flaky to throw OOM.
By considering the original work #6763 that added this test, the test can be against OpenHashSetSuite. And by doing this should be to save memory because OpenHashMap allocates two more arrays when growing the map/set.

How was this patch tested?

Existing tests.

viirya · 2018-09-27T10:10:05Z

cc @dongjoon-hyun @cloud-fan

cloud-fan · 2018-09-27T11:26:05Z

LGTM

SparkQA · 2018-09-27T13:53:14Z

Test build #96688 has finished for PR 22569 at commit 39a77d6.

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

viirya · 2018-09-27T13:56:02Z

retest this please.

kiszk · 2018-09-27T14:47:04Z

core/src/test/scala/org/apache/spark/util/collection/OpenHashSetSuite.scala

+      val pos1 = set.addWithoutResize(i) & OpenHashSet.POSITION_MASK
+      val pos2 = set.getPos(i)
+      assert(pos1 == pos2)
+    }


nit: Is it better to add the following to check each value after adding all, too?

for (i <- 0 until cnt) { assert(set.contains(i)) }

If we want to add the check, we can also add it inside the loop. Another loop seems unnecessary to me.

My intention was to verify whether small values (e.g. 0, 1, 2, 3) are still valid after several resizings.

I see. I think that is not what this wants test and there are other tests to make sure it works. If you insist I can add it too.

The original test performed a check by using map.iterator.count(). But, this is not a strong preference.

oh, the original test performed the count check to see how many values are invalid. They are invalid values because the index is wrong due to wrong position mask in OpenHashSet.

This rewritten test tests directly the index of OpenHashSet.

kiszk · 2018-09-27T14:47:29Z

LGTM except one minor comment

dongjoon-hyun · 2018-09-27T16:52:38Z

core/src/test/scala/org/apache/spark/util/collection/OpenHashSetSuite.scala

+      assert(set.contains(i))
+
+      val pos1 = set.addWithoutResize(i) & OpenHashSet.POSITION_MASK
+      val pos2 = set.getPos(i)


Thank you for doing this, @viirya .

Shall we switch the line 267 and 266 because set.addWithoutResize(i) has side-effects like changing the underlying structure. Although the situation will not happen because we called addWithoutResize inside add at line 263. But, we had better get the comparison target value from the read-only function first.

dongjoon-hyun · 2018-09-27T17:34:38Z

Also, please fix the tag in the PR title from [SQL] to [CORE].

SparkQA · 2018-09-27T18:30:07Z

Test build #96697 has finished for PR 22569 at commit 39a77d6.

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

SparkQA · 2018-09-27T19:39:29Z

Test build #96701 has finished for PR 22569 at commit dca4e5c.

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

SparkQA · 2018-09-28T03:29:26Z

Test build #96726 has finished for PR 22569 at commit 22bfa80.

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

viirya · 2018-09-28T03:41:05Z

retest this please.

SparkQA · 2018-09-28T07:05:01Z

Test build #96733 has finished for PR 22569 at commit 22bfa80.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

viirya · 2018-09-28T07:10:33Z

retest this please.

SparkQA · 2018-09-28T11:58:05Z

Test build #96738 has finished for PR 22569 at commit 22bfa80.

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

dongjoon-hyun

+1, LGTM. Thank you again.

dongjoon-hyun · 2018-09-28T21:28:21Z

Merged to master/branch-2.4.

…HashSetSuite and make it against OpenHashSet ## What changes were proposed in this pull request? The specified test in OpenHashMapSuite to test large items is somehow flaky to throw OOM. By considering the original work #6763 that added this test, the test can be against OpenHashSetSuite. And by doing this should be to save memory because OpenHashMap allocates two more arrays when growing the map/set. ## How was this patch tested? Existing tests. Closes #22569 from viirya/SPARK-25542. Authored-by: Liang-Chi Hsieh <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]> (cherry picked from commit b7d8034) Signed-off-by: Dongjoon Hyun <[email protected]>

…HashSetSuite and make it against OpenHashSet ## What changes were proposed in this pull request? The specified test in OpenHashMapSuite to test large items is somehow flaky to throw OOM. By considering the original work apache#6763 that added this test, the test can be against OpenHashSetSuite. And by doing this should be to save memory because OpenHashMap allocates two more arrays when growing the map/set. ## How was this patch tested? Existing tests. Closes apache#22569 from viirya/SPARK-25542. Authored-by: Liang-Chi Hsieh <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

Move 12M capability test from OpenHashMapSuite to OpenHashSetSuite.

39a77d6

viirya changed the title ~~[SPARK-25542][SQL][Test] Move flaky test in OpenHashMapSuite to OpenHashSetSuite~~ [SPARK-25542][SQL][Test] Move flaky test in OpenHashMapSuite to OpenHashSetSuite and make it against OpenHashSet Sep 27, 2018

kiszk reviewed Sep 27, 2018

View reviewed changes

Add one more check.

dca4e5c

dongjoon-hyun reviewed Sep 27, 2018

View reviewed changes

viirya changed the title ~~[SPARK-25542][SQL][Test] Move flaky test in OpenHashMapSuite to OpenHashSetSuite and make it against OpenHashSet~~ [SPARK-25542][Core][Test] Move flaky test in OpenHashMapSuite to OpenHashSetSuite and make it against OpenHashSet Sep 27, 2018

Change for comment.

22bfa80

dongjoon-hyun approved these changes Sep 28, 2018

View reviewed changes

asfgit closed this in b7d8034 Sep 28, 2018

viirya deleted the SPARK-25542 branch December 27, 2023 18:35

[SPARK-25542][Core][Test] Move flaky test in OpenHashMapSuite to OpenHashSetSuite and make it against OpenHashSet #22569

[SPARK-25542][Core][Test] Move flaky test in OpenHashMapSuite to OpenHashSetSuite and make it against OpenHashSet #22569

Uh oh!

Conversation

viirya commented Sep 27, 2018

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

viirya commented Sep 27, 2018

Uh oh!

cloud-fan commented Sep 27, 2018

Uh oh!

SparkQA commented Sep 27, 2018

Uh oh!

viirya commented Sep 27, 2018

Uh oh!

kiszk Sep 27, 2018

Choose a reason for hiding this comment

Uh oh!

viirya Sep 27, 2018

Choose a reason for hiding this comment

Uh oh!

kiszk Sep 28, 2018

Choose a reason for hiding this comment

Uh oh!

viirya Sep 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kiszk Sep 28, 2018

Choose a reason for hiding this comment

Uh oh!

viirya Sep 28, 2018

Choose a reason for hiding this comment

Uh oh!

kiszk commented Sep 27, 2018

Uh oh!

dongjoon-hyun Sep 27, 2018

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Sep 27, 2018

Uh oh!

SparkQA commented Sep 27, 2018

Uh oh!

SparkQA commented Sep 27, 2018

Uh oh!

SparkQA commented Sep 28, 2018

Uh oh!

viirya commented Sep 28, 2018

Uh oh!

SparkQA commented Sep 28, 2018

Uh oh!

viirya commented Sep 28, 2018

Uh oh!

SparkQA commented Sep 28, 2018

Uh oh!

dongjoon-hyun left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Sep 28, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

viirya Sep 28, 2018 •

edited

Loading

dongjoon-hyun left a comment •

edited

Loading