[SPARK-32900][CORE] Allow UnsafeExternalSorter to spill when there are nulls. #29772

tomvanbussel · 2020-09-16T11:58:50Z

What changes were proposed in this pull request?

This PR changes the way UnsafeExternalSorter.SpillableIterator checks whether it has spilled already, by checking whether inMemSorter is null. It also allows it to spill other UnsafeSorterIterators than UnsafeInMemorySorter.SortedIterator.

Why are the changes needed?

Before this PR UnsafeExternalSorter.SpillableIterator could not spill when there are NULLs in the input and radix sorting is used. Currently, Spark determines whether UnsafeExternalSorter.SpillableIterator has not spilled yet by checking whether upstream is an instance of UnsafeInMemorySorter.SortedIterator. When radix sorting is used and there are NULLs in the input however, upstream will be an instance of UnsafeExternalSorter.ChainedIterator instead, and Spark will assume that the SpillableIterator iterator has spilled already, and therefore cannot spill again when it's supposed to spill.

Does this PR introduce any user-facing change?

No

How was this patch tested?

A test was added to UnsafeExternalSorterSuite (and therefore also to UnsafeExternalSorterRadixSortSuite). I manually confirmed that the test failed in UnsafeExternalSorterRadixSortSuite without this patch.

…e nulls.

tomvanbussel · 2020-09-16T11:59:19Z

cc @hvanhovell @cloud-fan

hvanhovell · 2020-09-16T12:36:51Z

ok to test

core/src/test/java/org/apache/spark/util/collection/unsafe/sort/UnsafeExternalSorterSuite.java

hvanhovell

LGTM pending jenkins

cloud-fan · 2020-09-16T14:55:03Z

core/src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeExternalSorter.java

    public long spill() throws IOException {
      synchronized (this) {
-        if (!(upstream instanceof UnsafeInMemorySorter.SortedIterator && nextUpstream == null
-          && numRecords > 0)) {
+        if (inMemSorter == null || numRecords <= 0) {


I haven't touched this file for a long time. Do you mind providing more context? Does inMemSorter == null || numRecords <= 0 mean this sorter has not been inserted any records yet?

I think it was originally supposed to mean this, but it could also mean that all records have been read. This is actually a bug as during the call to spill it would prevent freeing the memory that's no longer necessary. I've kept this bug here to keep the changelist as small as possible (there's a little bit more to this than just removing the check). I'll address this issue in a separate ticket.

SparkQA · 2020-09-16T15:16:16Z

Test build #128768 has finished for PR 29772 at commit e6dbe2b.

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

SparkQA · 2020-09-16T15:42:55Z

Test build #128769 has finished for PR 29772 at commit 6eaf7f0.

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

cloud-fan · 2020-09-16T16:35:57Z

retest this please

SparkQA · 2020-09-16T17:07:53Z

Test build #128770 has finished for PR 29772 at commit 4bfa122.

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

SparkQA · 2020-09-16T17:16:34Z

Test build #128771 has finished for PR 29772 at commit e514327.

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

SparkQA · 2020-09-16T19:06:27Z

Test build #128776 has finished for PR 29772 at commit e514327.

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

viirya · 2020-09-17T05:07:39Z

Currently, Spark determines whether UnsafeExternalSorter.SpillableIterator has spilled already by checking whether upstream is an instance of UnsafeInMemorySorter.SortedIterator

Can we update the description a bit?

This is reading like Spark thinks UnsafeExternalSorter.SpillableIterator has spilled already if upstream is UnsafeInMemorySorter.SortedIterator. But it actually is Spark thinks UnsafeExternalSorter.SpillableIterator has spilled already if upstream is not UnsafeInMemorySorter.SortedIterator, right?

if (!(upstream instanceof UnsafeInMemorySorter.SortedIterator && nextUpstream == null
  && numRecords > 0)) {
  return 0L;
}

tomvanbussel · 2020-09-17T09:27:59Z

Thanks for the review! Reworded the description to be clearer.

hvanhovell · 2020-09-17T10:34:36Z

Alright merging to master/3.0/2.4. Thanks!

…e nulls ### What changes were proposed in this pull request? This PR changes the way `UnsafeExternalSorter.SpillableIterator` checks whether it has spilled already, by checking whether `inMemSorter` is null. It also allows it to spill other `UnsafeSorterIterator`s than `UnsafeInMemorySorter.SortedIterator`. ### Why are the changes needed? Before this PR `UnsafeExternalSorter.SpillableIterator` could not spill when there are NULLs in the input and radix sorting is used. Currently, Spark determines whether UnsafeExternalSorter.SpillableIterator has not spilled yet by checking whether `upstream` is an instance of `UnsafeInMemorySorter.SortedIterator`. When radix sorting is used and there are NULLs in the input however, `upstream` will be an instance of `UnsafeExternalSorter.ChainedIterator` instead, and Spark will assume that the `SpillableIterator` iterator has spilled already, and therefore cannot spill again when it's supposed to spill. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? A test was added to `UnsafeExternalSorterSuite` (and therefore also to `UnsafeExternalSorterRadixSortSuite`). I manually confirmed that the test failed in `UnsafeExternalSorterRadixSortSuite` without this patch. Closes #29772 from tomvanbussel/SPARK-32900. Authored-by: Tom van Bussel <tom.vanbussel@databricks.com> Signed-off-by: herman <herman@databricks.com> (cherry picked from commit e5e54a3) Signed-off-by: herman <herman@databricks.com>

…e nulls ### What changes were proposed in this pull request? This PR changes the way `UnsafeExternalSorter.SpillableIterator` checks whether it has spilled already, by checking whether `inMemSorter` is null. It also allows it to spill other `UnsafeSorterIterator`s than `UnsafeInMemorySorter.SortedIterator`. ### Why are the changes needed? Before this PR `UnsafeExternalSorter.SpillableIterator` could not spill when there are NULLs in the input and radix sorting is used. Currently, Spark determines whether UnsafeExternalSorter.SpillableIterator has not spilled yet by checking whether `upstream` is an instance of `UnsafeInMemorySorter.SortedIterator`. When radix sorting is used and there are NULLs in the input however, `upstream` will be an instance of `UnsafeExternalSorter.ChainedIterator` instead, and Spark will assume that the `SpillableIterator` iterator has spilled already, and therefore cannot spill again when it's supposed to spill. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? A test was added to `UnsafeExternalSorterSuite` (and therefore also to `UnsafeExternalSorterRadixSortSuite`). I manually confirmed that the test failed in `UnsafeExternalSorterRadixSortSuite` without this patch. Closes apache#29772 from tomvanbussel/SPARK-32900. Authored-by: Tom van Bussel <tom.vanbussel@databricks.com> Signed-off-by: herman <herman@databricks.com> (cherry picked from commit e5e54a3) Signed-off-by: herman <herman@databricks.com>

cxzl25 · 2021-07-07T13:10:35Z

Recently, I encountered this problem in the production environment. There are millions of nulls in the memory. There is no way to spill.
I used your patch and the verification took effect.
Thank you. @tomvanbussel
Before this, I also raised related questions. #26323

…e nulls This PR changes the way `UnsafeExternalSorter.SpillableIterator` checks whether it has spilled already, by checking whether `inMemSorter` is null. It also allows it to spill other `UnsafeSorterIterator`s than `UnsafeInMemorySorter.SortedIterator`. Before this PR `UnsafeExternalSorter.SpillableIterator` could not spill when there are NULLs in the input and radix sorting is used. Currently, Spark determines whether UnsafeExternalSorter.SpillableIterator has not spilled yet by checking whether `upstream` is an instance of `UnsafeInMemorySorter.SortedIterator`. When radix sorting is used and there are NULLs in the input however, `upstream` will be an instance of `UnsafeExternalSorter.ChainedIterator` instead, and Spark will assume that the `SpillableIterator` iterator has spilled already, and therefore cannot spill again when it's supposed to spill. No A test was added to `UnsafeExternalSorterSuite` (and therefore also to `UnsafeExternalSorterRadixSortSuite`). I manually confirmed that the test failed in `UnsafeExternalSorterRadixSortSuite` without this patch. Closes apache#29772 from tomvanbussel/SPARK-32900. Authored-by: Tom van Bussel <tom.vanbussel@databricks.com> Signed-off-by: herman <herman@databricks.com> (cherry picked from commit e5e54a3) Signed-off-by: herman <herman@databricks.com> Ref: LIHADOOP-58062 RB=2553974 BUG=LIHADOOP-58062 G=spark-reviewers R=mmuralid A=mmuralid

tomvanbussel added 2 commits September 16, 2020 13:43

[SPARK-32900][CORE] Allow UnsafeExternalSorter to spill when there ar…

f1440c4

…e nulls.

Add a test

e6dbe2b

probot-autolabeler bot added the CORE label Sep 16, 2020

tomvanbussel added 2 commits September 16, 2020 15:04

Fix

6eaf7f0

Fix

4bfa122

hvanhovell reviewed Sep 16, 2020

View reviewed changes

core/src/test/java/org/apache/spark/util/collection/unsafe/sort/UnsafeExternalSorterSuite.java Outdated Show resolved Hide resolved

Include some non-null values in the test

e514327

hvanhovell approved these changes Sep 16, 2020

View reviewed changes

cloud-fan reviewed Sep 16, 2020

View reviewed changes

viirya approved these changes Sep 17, 2020

View reviewed changes

hvanhovell closed this in e5e54a3 Sep 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-32900][CORE] Allow UnsafeExternalSorter to spill when there are nulls. #29772

[SPARK-32900][CORE] Allow UnsafeExternalSorter to spill when there are nulls. #29772

tomvanbussel commented Sep 16, 2020 •

edited

Loading

tomvanbussel commented Sep 16, 2020

hvanhovell commented Sep 16, 2020

hvanhovell left a comment

cloud-fan Sep 16, 2020

tomvanbussel Sep 16, 2020 •

edited

Loading

SparkQA commented Sep 16, 2020

SparkQA commented Sep 16, 2020

cloud-fan commented Sep 16, 2020

SparkQA commented Sep 16, 2020

SparkQA commented Sep 16, 2020

SparkQA commented Sep 16, 2020

viirya commented Sep 17, 2020

tomvanbussel commented Sep 17, 2020

hvanhovell commented Sep 17, 2020

cxzl25 commented Jul 7, 2021

[SPARK-32900][CORE] Allow UnsafeExternalSorter to spill when there are nulls. #29772

[SPARK-32900][CORE] Allow UnsafeExternalSorter to spill when there are nulls. #29772

Conversation

tomvanbussel commented Sep 16, 2020 • edited Loading

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

tomvanbussel commented Sep 16, 2020

hvanhovell commented Sep 16, 2020

hvanhovell left a comment

Choose a reason for hiding this comment

cloud-fan Sep 16, 2020

Choose a reason for hiding this comment

tomvanbussel Sep 16, 2020 • edited Loading

Choose a reason for hiding this comment

SparkQA commented Sep 16, 2020

SparkQA commented Sep 16, 2020

cloud-fan commented Sep 16, 2020

SparkQA commented Sep 16, 2020

SparkQA commented Sep 16, 2020

SparkQA commented Sep 16, 2020

viirya commented Sep 17, 2020

tomvanbussel commented Sep 17, 2020

hvanhovell commented Sep 17, 2020

cxzl25 commented Jul 7, 2021

tomvanbussel commented Sep 16, 2020 •

edited

Loading

tomvanbussel Sep 16, 2020 •

edited

Loading