-
Notifications
You must be signed in to change notification settings - Fork 28.9k
[SPARK-9303] Decimal should use java.math.Decimal directly instead of using Scala wrapper #7635
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
… via Scala wrapper
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we remove this entirely?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it considered a public API?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Decimal is not meant to be public. I think it just accidentally became public.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think user's code should never touch Decimal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're in agreement that it should be removed, then I'm glad to update this patch to do so (which involves adding a MiMa exclusion, I think). We could also just leave it and decide later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should remove it, but it is ok to leave it in for now I think. Most likely this entire class will look very different soon...
|
Test build #38319 has finished for PR 7635 at commit
|
|
For my own reference, here's the list of failed tests: |
|
Some of these tests are failing due to legitimate issues with Decimal hashCodes. Since this needs more design work to figure out the right semantics, I'm going to close this and un-assign the JIRA ticket. |
Spark SQL's
Decimalclass should use Java's BigDecimal instead of Scala's, since this removes a layer of object allocation and works around an issue where Scala's BigDecimal.hashCode() can lead to OOMs (see https://issues.scala-lang.org/browse/SI-6173).