Skip to content

Conversation

@kiszk
Copy link
Member

@kiszk kiszk commented Dec 9, 2016

What changes were proposed in this pull request?

This PR avoids that a result of a cast toInt is negative due to signed integer overflow (e.g. 0x0000_0000_1???????L.toInt < 0 ). This PR performs casts after we can ensure the value is within range of signed integer (the result of max(array.length, ???) is always integer).

How was this patch tested?

Manually executed query68 of TPC-DS with 100TB

@hvanhovell
Copy link
Contributor

LGTM - pending jenkins.

@kiszk do you think there is a sane way of testing this?

@hvanhovell
Copy link
Contributor

also cc @davies

@SparkQA
Copy link

SparkQA commented Dec 9, 2016

Test build #69926 has finished for PR 16235 at commit 7d2782f.

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

@hvanhovell
Copy link
Contributor

retest this please

@davies
Copy link
Contributor

davies commented Dec 9, 2016

LGTM, thanks for fixing it!

@kiszk
Copy link
Member Author

kiszk commented Dec 9, 2016

@hvanhovell if we want to test these methods, it seems to be complicated since we have to allocate large array by a naive approach that allocates LongToUnsafeRowMap and expands array.
Another approach is to directly call private methods writeLongArray and readLongArray by using reflection.

@hvanhovell
Copy link
Contributor

Ok, lets not do that for now.

@hvanhovell
Copy link
Contributor

It might at some point be an idea to add a bunch of asserts to Platform, that would make diagnoses a lot easier.

@SparkQA
Copy link

SparkQA commented Dec 9, 2016

Test build #69928 has finished for PR 16235 at commit 7d2782f.

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

@kiszk
Copy link
Member Author

kiszk commented Dec 9, 2016

Jenkins, retest this please

@SparkQA
Copy link

SparkQA commented Dec 9, 2016

Test build #69930 has finished for PR 16235 at commit 7d2782f.

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

@hvanhovell
Copy link
Contributor

hvanhovell commented Dec 9, 2016

Merging to master/2.1/2.0. Thanks!

asfgit pushed a commit that referenced this pull request Dec 9, 2016
## What changes were proposed in this pull request?

This PR avoids that a result of a cast `toInt` is negative due to signed integer overflow (e.g. 0x0000_0000_1???????L.toInt < 0 ). This PR performs casts after we can ensure the value is within range of signed integer (the result of `max(array.length, ???)` is always integer).

## How was this patch tested?

Manually executed query68 of TPC-DS with 100TB

Author: Kazuaki Ishizaki <[email protected]>

Closes #16235 from kiszk/SPARK-18745.

(cherry picked from commit d60ab5f)
Signed-off-by: Herman van Hovell <[email protected]>
asfgit pushed a commit that referenced this pull request Dec 9, 2016
## What changes were proposed in this pull request?

This PR avoids that a result of a cast `toInt` is negative due to signed integer overflow (e.g. 0x0000_0000_1???????L.toInt < 0 ). This PR performs casts after we can ensure the value is within range of signed integer (the result of `max(array.length, ???)` is always integer).

## How was this patch tested?

Manually executed query68 of TPC-DS with 100TB

Author: Kazuaki Ishizaki <[email protected]>

Closes #16235 from kiszk/SPARK-18745.

(cherry picked from commit d60ab5f)
Signed-off-by: Herman van Hovell <[email protected]>
@asfgit asfgit closed this in d60ab5f Dec 9, 2016
robert3005 pushed a commit to palantir/spark that referenced this pull request Dec 15, 2016
## What changes were proposed in this pull request?

This PR avoids that a result of a cast `toInt` is negative due to signed integer overflow (e.g. 0x0000_0000_1???????L.toInt < 0 ). This PR performs casts after we can ensure the value is within range of signed integer (the result of `max(array.length, ???)` is always integer).

## How was this patch tested?

Manually executed query68 of TPC-DS with 100TB

Author: Kazuaki Ishizaki <[email protected]>

Closes apache#16235 from kiszk/SPARK-18745.
uzadude pushed a commit to uzadude/spark that referenced this pull request Jan 27, 2017
## What changes were proposed in this pull request?

This PR avoids that a result of a cast `toInt` is negative due to signed integer overflow (e.g. 0x0000_0000_1???????L.toInt < 0 ). This PR performs casts after we can ensure the value is within range of signed integer (the result of `max(array.length, ???)` is always integer).

## How was this patch tested?

Manually executed query68 of TPC-DS with 100TB

Author: Kazuaki Ishizaki <[email protected]>

Closes apache#16235 from kiszk/SPARK-18745.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants