-
Notifications
You must be signed in to change notification settings - Fork 853
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
Support decimal negative scale #3152
Conversation
f880bd4
to
a782f91
Compare
a782f91
to
0ae98a8
Compare
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 we probably want to do a quick parse over the codebase for places that assume the scale is unsigned.
Doing a search for scale as
shows a couple of scale as usize
which are likely incorrect
@@ -1039,7 +1039,14 @@ impl<T: DecimalType + ArrowPrimitiveType> PrimitiveArray<T> { | |||
Decimal128Type::MAX_SCALE | |||
))); | |||
} | |||
if scale > precision { | |||
if scale < -T::MAX_SCALE { |
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 wasn't sure about this, as the rationale for enforcing the upper limit is if the positive scale exceeds the precision it would result in truncation. I'm not sure this applies to the negative direction. However, the C++ version has this same check, so 🤷 - https://github.com/apache/arrow/blob/91ee6dad722ee154d63eea86ce5644e1e658b53b/cpp/src/arrow/util/decimal.cc#L389
These Other Another two is The problematic one is to convert |
when I first meet the negative scale, I was confused by this. |
Benchmark runs are scheduled for baseline = 6c466af and contender = 78ab0ef. 78ab0ef is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
Closes #1785.
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?