[SPARK-15445][SQL] Build fails for java 1.7 after adding java.mathBigInteger support #13223

techaddict · 2016-05-20T11:09:26Z

What changes were proposed in this pull request?

Using longValue() and then checking whether the value is in the range for a long manually.

How was this patch tested?

Existing tests

…h.BigInteger support

techaddict · 2016-05-20T11:10:32Z

cc: @srowen

srowen · 2016-05-20T11:13:37Z

sql/catalyst/src/main/scala/org/apache/spark/sql/types/Decimal.scala

      this.decimalVal = null
-      this.longVal = bigintval.longValueExact()
+      // TODO: Remove this once we migrate to java8 and use longValueExact() instead.
+      val isExactLong = bigintval.compareTo(BigInteger.valueOf(JLong.MAX_VALUE)) <= 0 &&


Maybe extract constants for a BigInteger representation of the max/min values rather than compute each time? Also is this all uses of java.util.Long that can be JLong?

srowen · 2016-05-20T12:09:31Z

sql/catalyst/src/main/scala/org/apache/spark/sql/types/Decimal.scala

  def toScalaBigInt: BigInt = BigInt(toLong)

-  def toJavaBigInteger: java.math.BigInteger = java.math.BigInteger.valueOf(toLong)
+  def toJavaBigInteger: BigInteger = BigInteger.valueOf(toLong)


Although I support removing the redundant qualified name, I wonder if we should also un-qualify scala.math.BigInt, or else leave both qualified even though they're not ambiguous. BigDecimal is used unqualified while the Java version is always qualified, to disambiguate. I guess it would become consistent if BigInt were not qualified.

SparkQA · 2016-05-20T12:24:05Z

Test build #58977 has finished for PR 13223 at commit 29f5adb.

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

SparkQA · 2016-05-20T12:34:43Z

Test build #58978 has finished for PR 13223 at commit c0766f0.

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

SparkQA · 2016-05-20T12:44:33Z

Test build #58979 has finished for PR 13223 at commit a1e54c2.

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

SparkQA · 2016-05-20T12:54:45Z

Test build #58980 has finished for PR 13223 at commit 90f2cdc.

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

SparkQA · 2016-05-20T13:52:17Z

Test build #58987 has finished for PR 13223 at commit e5a5839.

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

techaddict · 2016-05-20T14:50:32Z

@srowen Comments addressed.

gatorsmile · 2016-05-20T15:25:38Z

cc @kevinyu98

kevinyu98 · 2016-05-20T15:35:45Z

@techaddict @srowen @cloud-fan @gatorsmile : Hi Sandeep , thanks for fixing this. I didn't realize the method is java 1.8 only. The code looks good to me.

rxin · 2016-05-20T19:09:04Z

sql/catalyst/src/main/scala/org/apache/spark/sql/types/Decimal.scala

      case d: BigDecimal => apply(d)
-      case k: scala.math.BigInt => apply(k)
-      case l: java.math.BigInteger => apply(l)
+      case k: BigInt => apply(k)


this is too confusing. please keep the qualifiers. i'd actually keep qualifiers everywhere.

The code is inconsistent in this regard though. I thought it worth cleaning up but don't feel strongly about it. Alternative is to qualify both classes everywhere consistently

I agree it's better to make it consistent, but let's not mix it into a simple fix for java 7 ...

rxin · 2016-05-20T19:10:08Z

@techaddict thanks for doing this. Can you be more surgical in fixes like this? i.e. just fix the line that is causing problem.

rxin · 2016-05-20T19:11:24Z

Also I think you can remove "HOTFIX" -- those are for master build breaking changes, which isn't happening here yet. (fwiw we don't run java 7 on master)

rxin · 2016-05-20T19:48:43Z

sql/catalyst/src/main/scala/org/apache/spark/sql/types/Decimal.scala

  private[sql] val ZERO = Decimal(0)
  private[sql] val ONE = Decimal(1)

+  private[types] val LONG_MAX_BIG_INT = BigInteger.valueOf(JLong.MAX_VALUE)


this can just be private

SparkQA · 2016-05-20T21:29:54Z

Test build #59022 has finished for PR 13223 at commit 35da7c2.

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

techaddict · 2016-05-21T00:18:11Z

@rxin @srowen I've added qualifiers back.

SparkQA · 2016-05-21T01:48:04Z

Test build #59048 has finished for PR 13223 at commit faff787.

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

srowen · 2016-05-21T11:40:00Z

Merged to master/2.0

…Integer support ## What changes were proposed in this pull request? Using longValue() and then checking whether the value is in the range for a long manually. ## How was this patch tested? Existing tests Author: Sandeep Singh <[email protected]> Closes #13223 from techaddict/SPARK-15445. (cherry picked from commit 666bf2e) Signed-off-by: Sean Owen <[email protected]>

techaddict added 3 commits May 20, 2016 16:31

[SPARK-15445] [HOTFIX] Build fails for java 1.7 after adding java.mat…

02a1fef

…h.BigInteger support

fix values

cd8362d

fix

29f5adb

srowen reviewed May 20, 2016
View reviewed changes

techaddict added 4 commits May 20, 2016 16:48

fix tests

c0766f0

max constants

68937ce

remove try catch

a1e54c2

since we import BigInteger

90f2cdc

srowen reviewed May 20, 2016
View reviewed changes

techaddict added 2 commits May 20, 2016 17:47

Merge branch 'master' into SPARK-15445

d9429da

use require instead of throwing exception

b65715d

un-qualify scala.math.BigInt

e5a5839

rxin reviewed May 20, 2016
View reviewed changes

techaddict added 2 commits May 21, 2016 01:24

Merge branch 'master' into SPARK-15445

86eb2b3

make long min max private

35da7c2

techaddict changed the title ~~[HOTFIX][SPARK-15445] Build fails for java 1.7 after adding java.mathBigInteger support~~ [SPARK-15445][SQL] Build fails for java 1.7 after adding java.mathBigInteger support May 20, 2016

srowen mentioned this pull request May 20, 2016

[SPARK-11827][SQL] Fix compilation against Java 7 #13233

Closed

keep qualifiers

faff787

asfgit closed this in 666bf2e May 21, 2016

[SPARK-15445][SQL] Build fails for java 1.7 after adding java.mathBigInteger support #13223

[SPARK-15445][SQL] Build fails for java 1.7 after adding java.mathBigInteger support #13223

Uh oh!

Conversation

techaddict commented May 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

techaddict commented May 20, 2016

Uh oh!

srowen May 20, 2016

Choose a reason for hiding this comment

Uh oh!

srowen May 20, 2016

Choose a reason for hiding this comment

Uh oh!

SparkQA commented May 20, 2016

Uh oh!

SparkQA commented May 20, 2016

Uh oh!

SparkQA commented May 20, 2016

Uh oh!

SparkQA commented May 20, 2016

Uh oh!

SparkQA commented May 20, 2016

Uh oh!

techaddict commented May 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gatorsmile commented May 20, 2016

Uh oh!

kevinyu98 commented May 20, 2016

Uh oh!

rxin May 20, 2016

Choose a reason for hiding this comment

Uh oh!

srowen May 20, 2016

Choose a reason for hiding this comment

Uh oh!

rxin May 20, 2016

Choose a reason for hiding this comment

Uh oh!

rxin commented May 20, 2016

Uh oh!

rxin commented May 20, 2016

Uh oh!

rxin May 20, 2016

Choose a reason for hiding this comment

Uh oh!

SparkQA commented May 20, 2016

Uh oh!

techaddict commented May 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented May 21, 2016

Uh oh!

srowen commented May 21, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

techaddict commented May 20, 2016 •

edited

Loading

techaddict commented May 20, 2016 •

edited

Loading

techaddict commented May 21, 2016 •

edited

Loading