You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/11/19 07:58:43 UTC
[GitHub] [arrow-rs] liukun4515 opened a new pull request, #3139: Cast: should get the round result for decimal to a decimal with smaller scale
liukun4515 opened a new pull request, #3139:
URL: https://github.com/apache/arrow-rs/pull/3139
# Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes https://github.com/apache/arrow-rs/issues/3137
# Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
# What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
# Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!---
If there are any breaking changes to public APIs, please add the `breaking change` label.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] liukun4515 commented on pull request #3139: Cast: should get the round result for decimal to a decimal with smaller scale
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on PR #3139:
URL: https://github.com/apache/arrow-rs/pull/3139#issuecomment-1327945474
the decimal(10,-1) with the 128-bit integer (123), the string of the value is `1230`, if we cast it to the decimal(10,-2), what the 128-bit integer of result should be? @tustvold @viirya
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] liukun4515 commented on pull request #3139: Cast: should get the round result for decimal to a decimal with smaller scale
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on PR #3139:
URL: https://github.com/apache/arrow-rs/pull/3139#issuecomment-1327944556
Maybe I got your thought from this commit https://github.com/apache/arrow-rs/pull/3139/commits/2abbf8901f0a1fe48af99c9f9a85b5de094647c1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] liukun4515 commented on pull request #3139: Cast: should get the round result for decimal to a decimal with smaller scale
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on PR #3139:
URL: https://github.com/apache/arrow-rs/pull/3139#issuecomment-1322929369
> I think this should consistently use wrapping or checked add, neg, div, rem, etc... This not only is consistent with other kernels, but avoids differences between release and debug builds
The changes i have done will not overflow.
It's good to make consistent with other kernel.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] liukun4515 commented on pull request #3139: Cast: should get the round result for decimal to a decimal with smaller scale
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on PR #3139:
URL: https://github.com/apache/arrow-rs/pull/3139#issuecomment-1328150981
> 123
I am confused about this, if the data type is decimal(10,-2) and the 128-bit integer is `123`, it represent the value of `12300`, and the value has been changed after casting.
I think the 128-bit integer should be `12` after casted to decimal(10,-2).
From the doc: https://arrow.apache.org/docs/python/generated/pyarrow.decimal128.html#pyarrow-decimal128
```
decimal128(5, -3) can exactly represent the number 12345000 (encoded internally as the 128-bit integer 12345), but neither 123450000 nor 1234500.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #3139: Cast: should get the round result for decimal to a decimal with smaller scale
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #3139:
URL: https://github.com/apache/arrow-rs/pull/3139#discussion_r1028299154
##########
arrow-cast/src/cast.rs:
##########
@@ -1955,12 +1956,26 @@ fn cast_decimal_to_decimal_safe<const BYTE_WIDTH1: usize, const BYTE_WIDTH2: usi
// For example, input_scale is 4 and output_scale is 3;
// Original value is 11234_i128, and will be cast to 1123_i128.
let div = 10_i128.pow((input_scale - output_scale) as u32);
+ let half = div / 2;
+ let neg_half = half.neg();
if BYTE_WIDTH1 == 16 {
let array = array.as_any().downcast_ref::<Decimal128Array>().unwrap();
if BYTE_WIDTH2 == 16 {
- let iter = array
- .iter()
- .map(|v| v.and_then(|v| v.div_checked(div).ok()));
+ // rounding the result
+ let iter = array.iter().map(|v| {
+ v.map(|v| {
+ // the div must be gt_eq 10, we don't need to check the overflow for the `div`/`mod` operation
+ let d = v / div;
+ let r = v % div;
Review Comment:
```suggestion
let d = v.wrapping_div(div);
let r = v.wrapping_rem(div);
```
##########
arrow-cast/src/cast.rs:
##########
@@ -1955,12 +1956,26 @@ fn cast_decimal_to_decimal_safe<const BYTE_WIDTH1: usize, const BYTE_WIDTH2: usi
// For example, input_scale is 4 and output_scale is 3;
// Original value is 11234_i128, and will be cast to 1123_i128.
let div = 10_i128.pow((input_scale - output_scale) as u32);
+ let half = div / 2;
+ let neg_half = half.neg();
Review Comment:
```suggestion
let neg_half = half.wrapping_neg();
```
As we've divided by 2 this can't overflow
##########
arrow-cast/src/cast.rs:
##########
@@ -1955,12 +1956,26 @@ fn cast_decimal_to_decimal_safe<const BYTE_WIDTH1: usize, const BYTE_WIDTH2: usi
// For example, input_scale is 4 and output_scale is 3;
// Original value is 11234_i128, and will be cast to 1123_i128.
let div = 10_i128.pow((input_scale - output_scale) as u32);
+ let half = div / 2;
+ let neg_half = half.neg();
if BYTE_WIDTH1 == 16 {
let array = array.as_any().downcast_ref::<Decimal128Array>().unwrap();
if BYTE_WIDTH2 == 16 {
- let iter = array
- .iter()
- .map(|v| v.and_then(|v| v.div_checked(div).ok()));
+ // rounding the result
+ let iter = array.iter().map(|v| {
+ v.map(|v| {
+ // the div must be gt_eq 10, we don't need to check the overflow for the `div`/`mod` operation
+ let d = v / div;
+ let r = v % div;
+ if v >= 0 && r >= half {
+ d + 1
Review Comment:
```suggestion
d.wrapping_add(1)
```
##########
arrow-cast/src/cast.rs:
##########
@@ -1955,12 +1956,26 @@ fn cast_decimal_to_decimal_safe<const BYTE_WIDTH1: usize, const BYTE_WIDTH2: usi
// For example, input_scale is 4 and output_scale is 3;
// Original value is 11234_i128, and will be cast to 1123_i128.
let div = 10_i128.pow((input_scale - output_scale) as u32);
+ let half = div / 2;
+ let neg_half = half.neg();
if BYTE_WIDTH1 == 16 {
let array = array.as_any().downcast_ref::<Decimal128Array>().unwrap();
if BYTE_WIDTH2 == 16 {
- let iter = array
- .iter()
- .map(|v| v.and_then(|v| v.div_checked(div).ok()));
+ // rounding the result
+ let iter = array.iter().map(|v| {
+ v.map(|v| {
+ // the div must be gt_eq 10, we don't need to check the overflow for the `div`/`mod` operation
+ let d = v / div;
+ let r = v % div;
+ if v >= 0 && r >= half {
+ d + 1
+ } else if v < 0 && r <= neg_half {
+ d - 1
Review Comment:
```suggestion
d.wrapping_sub(1)
```
##########
arrow-cast/src/cast.rs:
##########
@@ -1981,9 +2006,23 @@ fn cast_decimal_to_decimal_safe<const BYTE_WIDTH1: usize, const BYTE_WIDTH2: usi
} else {
let array = array.as_any().downcast_ref::<Decimal256Array>().unwrap();
let div = i256::from_i128(div);
+ let half = div / i256::from_i128(2);
+ // TODO: it's better to implement the neg
+ let neg_half = half * i256::from_i128(-1);
Review Comment:
```suggestion
let neg_half = half.wrapping_neg();
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] liukun4515 commented on pull request #3139: Cast: should get the round result for decimal to a decimal with smaller scale
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on PR #3139:
URL: https://github.com/apache/arrow-rs/pull/3139#issuecomment-1327943489
> I think there is a logical conflict with one of the tests for negative scales
hi @tustvold Can you give an example to explain the conflict.
From https://github.com/apache/arrow-rs/pull/3152, I know negative scala is supported in the Arrow.
Before this, I have not known the usage of negative scale.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on pull request #3139: Cast: should get the round result for decimal to a decimal with smaller scale
Posted by GitBox <gi...@apache.org>.
tustvold commented on PR #3139:
URL: https://github.com/apache/arrow-rs/pull/3139#issuecomment-1328191081
Apologies I misread your example, if the integer value was `1230` casting would yield an integer value of `123`, with the same string value. Casting an integer value of `123` with a corresponding string value of `1230` I would expect to result in an error, although #3203 would suggest something isn't quite right here yet
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on pull request #3139: Cast: should get the round result for decimal to a decimal with smaller scale
Posted by GitBox <gi...@apache.org>.
tustvold commented on PR #3139:
URL: https://github.com/apache/arrow-rs/pull/3139#issuecomment-1325082909
Do you intend to switch to explicitly using wrapping / checked operations to ensure consistent behaviour across debug and release, and to be consistent with the other kernels?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on pull request #3139: Cast: should get the round result for decimal to a decimal with smaller scale
Posted by GitBox <gi...@apache.org>.
tustvold commented on PR #3139:
URL: https://github.com/apache/arrow-rs/pull/3139#issuecomment-1328005205
123
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold merged pull request #3139: Cast: should get the round result for decimal to a decimal with smaller scale
Posted by GitBox <gi...@apache.org>.
tustvold merged PR #3139:
URL: https://github.com/apache/arrow-rs/pull/3139
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] liukun4515 commented on pull request #3139: Cast: should get the round result for decimal to a decimal with smaller scale
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on PR #3139:
URL: https://github.com/apache/arrow-rs/pull/3139#issuecomment-1326209525
> Do you intend to switch to explicitly using wrapping / checked operations to ensure consistent behaviour across debug and release, and to be consistent with the other kernels?
@tustvold
Sorry for the late reply, i forgot to push the changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] liukun4515 commented on pull request #3139: Cast: should get the round result for decimal to a decimal with smaller scale
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on PR #3139:
URL: https://github.com/apache/arrow-rs/pull/3139#issuecomment-1320829042
Now it just implement the case of decimal128 to decimal128.
If the method of implementation looks good to all, I will fill out other case and add more test cases
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #3139: Cast: should get the round result for decimal to a decimal with smaller scale
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #3139:
URL: https://github.com/apache/arrow-rs/pull/3139#discussion_r1028295853
##########
arrow-cast/src/cast.rs:
##########
@@ -1981,9 +2006,23 @@ fn cast_decimal_to_decimal_safe<const BYTE_WIDTH1: usize, const BYTE_WIDTH2: usi
} else {
let array = array.as_any().downcast_ref::<Decimal256Array>().unwrap();
let div = i256::from_i128(div);
+ let half = div / i256::from_i128(2);
+ // TODO: it's better to implement the neg
Review Comment:
https://github.com/apache/arrow-rs/pull/3151
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] ursabot commented on pull request #3139: Cast: should get the round result for decimal to a decimal with smaller scale
Posted by GitBox <gi...@apache.org>.
ursabot commented on PR #3139:
URL: https://github.com/apache/arrow-rs/pull/3139#issuecomment-1327097867
Benchmark runs are scheduled for baseline = 2c86895f3672af9a0d835204ccc03108d342361e and contender = 187bf619dfafccdb21cea6b2cecabd29daffc1e4. 187bf619dfafccdb21cea6b2cecabd29daffc1e4 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ec2-t3-xlarge-us-east-2] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/3ab77f6f9feb4364b134dd84ad79bf4b...3291ad42798942448f5fd5ef9a43ccbe/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on test-mac-arm] [test-mac-arm](https://conbench.ursa.dev/compare/runs/47d222b1a43e4e6abbea99ba45efa20f...f6cec89a0eef4c1c94f0fce8d68c9314/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ursa-i9-9960x] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/b116d94a80c6433aa015725320d61e87...866323429d164991a50d6b544cdcbdce/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ursa-thinkcentre-m75q] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/2dcf8dba189343aba01b109e5e740d56...f68cbc268bb6418297a15490f3e36775/)
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org