[SPARK-18458][CORE] Fix signed integer overflow problem at an expression in RadixSort.java #15907

kiszk · 2016-11-16T18:48:09Z

What changes were proposed in this pull request?

This PR avoids that a result of an expression is negative due to signed integer overflow (e.g. 0x10?????? * 8 < 0). This PR casts each operand to long before executing a calculation. Since the result is interpreted as long, the result of the expression is positive.

How was this patch tested?

Manually executed query82 of TPC-DS with 100TB

…cation

kiszk · 2016-11-16T18:51:54Z

@hvanhovell I found similar portions in RadixSort.java For example, at line 85.
Should we change these portions preventively while we do not have test suites, too?

hvanhovell · 2016-11-16T19:04:48Z

cc @ericl

hvanhovell · 2016-11-16T19:05:12Z

@kiszk yeah lets also address those

kiszk · 2016-11-16T19:19:08Z

@hvanhovell as far as I know, I addressed them in RadixSort.java

SparkQA · 2016-11-16T21:24:04Z

Test build #68728 has finished for PR 15907 at commit 3821a52.

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

SparkQA · 2016-11-16T22:02:13Z

Test build #68732 has finished for PR 15907 at commit 472c0c3.

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

rxin · 2016-11-16T23:00:34Z

Can you please have a more descriptive title? Presumably the pull request is not to add coredump.

kiszk · 2016-11-17T02:37:04Z

@rxin Sure, I did.

ericl · 2016-11-17T02:57:17Z

LGTM. I wonder if it's also worth changing all the int parameters to longs to be extra safe here with conversions, even if we know the values are bounded.

rxin · 2016-11-17T06:13:18Z

Yea it might be a good idea to just use longs.

kiszk · 2016-11-17T06:25:13Z

Ideally, it is a good idea. Do you want to do that change using this PR?

Another question is which scope do we apply such a change?

Only in methods that I applied changes to.
Only in 'RadixSort.java'
Beyond public methods ('sort()' and 'sortKeyPrefixArray()')

Obviously, 3. requires a lot of changes to many files.

What do you think?

rxin · 2016-11-17T07:15:50Z

I'm thinking 2. What do you think?

kiszk · 2016-11-17T07:51:38Z

It would be good.
In that case, is it better to insert explicit cast (from int to long) if a caller gives an int variable? This is why we want to explicitly express a specification of public APIs (sort() and sortKeyPrefixArray()).

ericl

How about keeping the very small numbers (0..7, 0..255) as ints, and everything else as a long.

ericl · 2016-11-17T20:17:59Z

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

    if (numRecords > 0) {
      long[][] counts = getCounts(array, numRecords, startByteIndex, endByteIndex);
-      for (int i = startByteIndex; i <= endByteIndex; i++) {
-        if (counts[i] != null) {
+      for (long i = startByteIndex; i <= endByteIndex; i++) {


This should probably not be a long, since it ranges from 0..7

ericl · 2016-11-17T20:18:23Z

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

@@ -40,28 +40,28 @@
   *         of always copying the data back to position zero for efficiency.
   */
  public static int sort(
-      LongArray array, int numRecords, int startByteIndex, int endByteIndex,
+      LongArray array, long numRecords, long startByteIndex, long endByteIndex,


I think we should keep the byte indices as ints, since they are in 0..7 and we assert these below.

Sure, I imagined that all of variables should be changed to long anyway.

SparkQA · 2016-11-17T20:46:02Z

Test build #68793 has finished for PR 15907 at commit ae6820a.

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

SparkQA · 2016-11-17T23:20:35Z

Test build #68798 has finished for PR 15907 at commit ef54e8a.

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

ericl · 2016-11-18T00:56:29Z

Looks good

rxin · 2016-11-18T00:57:34Z

sql/core/src/test/scala/org/apache/spark/sql/execution/benchmark/SortBenchmark.scala

@@ -80,7 +80,7 @@ class SortBenchmark extends BenchmarkBase {
      }
      val buf = new LongArray(MemoryBlock.fromLongArray(array))
      timer.startTiming()
-      RadixSort.sort(buf, size, 0, 7, false, false)
+      RadixSort.sort(buf, size.toLong, 0, 7, false, false)


why do we need these toLongs? Doesn't scala do the cast implicitly?

As I wrote before, I wanted to emphasis the difference of types between caller and callee.
It is OK to rely on implicit cast, done.

rxin · 2016-11-18T00:59:10Z

core/src/test/scala/org/apache/spark/util/collection/unsafe/sort/RadixSortSuite.scala

    var i = 0
-    val out = new Array[Long](length)
+    val out = new Array[Long](length.toInt)


I'd add a checkedCast function similar to Guava's: https://google.github.io/guava/releases/19.0/api/docs/com/google/common/primitives/Ints.html#checkedCast(long)

and use those for all the toInt downcasts. Otherwise it'd be annoying to get mysterious negative integer exceptions.

Great! done in these files.

Did you forget to push?

Sorry, I put this comment before pushed. Now, you can see it

rxin · 2016-11-18T00:59:26Z

core/src/main/java/org/apache/spark/shuffle/sort/ShuffleInMemorySorter.java

@@ -176,7 +176,7 @@ public ShuffleSorterIterator getSortedIterator() {
    int offset = 0;
    if (useRadixSort) {
      offset = RadixSort.sort(
-        array, pos,
+        array, (long)pos,


again doesn't java do the upcast implicitly?

It is OK to rely on implicit cast, done.

SparkQA · 2016-11-18T04:59:13Z

Test build #68819 has finished for PR 15907 at commit 022e5b3.

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

SparkQA · 2016-11-18T05:17:01Z

Test build #68820 has finished for PR 15907 at commit 3f9efdb.

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

SparkQA · 2016-11-18T05:25:08Z

Test build #68821 has finished for PR 15907 at commit f2e2079.

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

srowen · 2016-11-18T09:15:30Z

core/src/test/scala/org/apache/spark/util/collection/unsafe/sort/RadixSortSuite.scala

@@ -30,7 +32,7 @@ import org.apache.spark.util.collection.Sorter
 import org.apache.spark.util.random.XORShiftRandom

 class RadixSortSuite extends SparkFunSuite with Logging {
-  private val N = 10000  // scale this down for more readable results
+  private val N = 10000.toLong  // scale this down for more readable results


Write 10000L

Sure, I will do that

srowen · 2016-11-18T09:16:10Z

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

          inIndex = outIndex;
          outIndex = tmp;
        }
      }
    }
-    return inIndex;
+    return Ints.checkedCast(inIndex);


Rather than pull in a library method, how about require which lets you provide an actual error message?

It might make sense to add a very simple if check in the beginning of this function. I will do it when I merge this.

SparkQA · 2016-11-18T17:16:11Z

Test build #68855 has finished for PR 15907 at commit 591626e.

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

rxin · 2016-11-20T05:49:43Z

Merging in master/branch-2.1

…ion in RadixSort.java ## What changes were proposed in this pull request? This PR avoids that a result of an expression is negative due to signed integer overflow (e.g. 0x10?????? * 8 < 0). This PR casts each operand to `long` before executing a calculation. Since the result is interpreted as long, the result of the expression is positive. ## How was this patch tested? Manually executed query82 of TPC-DS with 100TB Author: Kazuaki Ishizaki <ishizaki@jp.ibm.com> Closes #15907 from kiszk/SPARK-18458. (cherry picked from commit d93b655) Signed-off-by: Reynold Xin <rxin@databricks.com>

…ion in RadixSort.java ## What changes were proposed in this pull request? This PR avoids that a result of an expression is negative due to signed integer overflow (e.g. 0x10?????? * 8 < 0). This PR casts each operand to `long` before executing a calculation. Since the result is interpreted as long, the result of the expression is positive. ## How was this patch tested? Manually executed query82 of TPC-DS with 100TB Author: Kazuaki Ishizaki <ishizaki@jp.ibm.com> Closes apache#15907 from kiszk/SPARK-18458.

avoid signed integer overflow (0x10?????? * 8) by using long multipli…

3821a52

…cation

avoid signed integer overflow

472c0c3

kiszk changed the title ~~[SPARK-18458][CORE] core dumped running Spark SQL on large data volume (100TB)~~ [SPARK-18458][CORE] Avoid signed integer overflow at an expression in RadixSort.java Nov 17, 2016

kiszk changed the title ~~[SPARK-18458][CORE] Avoid signed integer overflow at an expression in RadixSort.java~~ [SPARK-18458][CORE] Fix signed integer overflow at an expression in RadixSort.java Nov 17, 2016

kiszk changed the title ~~[SPARK-18458][CORE] Fix signed integer overflow at an expression in RadixSort.java~~ [SPARK-18458][CORE] Fix signed integer overflow problem at an expression in RadixSort.java Nov 17, 2016

replace int with long in RadixSort.java

ae6820a

ericl reviewed Nov 17, 2016

View reviewed changes

revert changes from int to long for variables related to byteIndex

ef54e8a

rxin reviewed Nov 18, 2016

View reviewed changes

kiszk added 3 commits November 18, 2016 11:16

addressed review comments

022e5b3

addressed review comments

3f9efdb

revert incorrect cast

f2e2079

srowen requested changes Nov 18, 2016

View reviewed changes

addressed review comment

591626e

asfgit closed this in d93b655 Nov 20, 2016

[SPARK-18458][CORE] Fix signed integer overflow problem at an expression in RadixSort.java #15907

[SPARK-18458][CORE] Fix signed integer overflow problem at an expression in RadixSort.java #15907

Uh oh!

Conversation

kiszk commented Nov 16, 2016

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

kiszk commented Nov 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hvanhovell commented Nov 16, 2016

Uh oh!

hvanhovell commented Nov 16, 2016

Uh oh!

kiszk commented Nov 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Nov 16, 2016

Uh oh!

SparkQA commented Nov 16, 2016

Uh oh!

rxin commented Nov 16, 2016

Uh oh!

kiszk commented Nov 17, 2016

Uh oh!

ericl commented Nov 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rxin commented Nov 17, 2016

Uh oh!

kiszk commented Nov 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rxin commented Nov 17, 2016

Uh oh!

kiszk commented Nov 17, 2016

Uh oh!

ericl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 17, 2016

Uh oh!

SparkQA commented Nov 17, 2016

Uh oh!

ericl commented Nov 18, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kiszk Nov 18, 2016 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kiszk Nov 18, 2016 • 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!

SparkQA commented Nov 18, 2016

Uh oh!

SparkQA commented Nov 18, 2016

Uh oh!

SparkQA commented Nov 18, 2016

Uh oh!

Choose a reason for hiding this comment

kiszk commented Nov 16, 2016 •

edited

Loading

kiszk commented Nov 16, 2016 •

edited

Loading

ericl commented Nov 17, 2016 •

edited

Loading

kiszk commented Nov 17, 2016 •

edited

Loading

kiszk Nov 18, 2016 •

edited

Loading

kiszk Nov 18, 2016 •

edited

Loading