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/08/08 13:04:36 UTC
[GitHub] [arrow-rs] HaoYang670 opened a new issue, #2362: The constructor `DataType::Decimal(usize, usize)` is unsound.
HaoYang670 opened a new issue, #2362:
URL: https://github.com/apache/arrow-rs/issues/2362
**Describe the bug**
Also related to the Decimal. Currently, the constructor DataType::Decimal128/256(precision: usize, scale: usize) is unsound, because users can put any value in it. Also this leads to 2 kinds of bug in the code:
1. forget to check the value of precision and scale
2. redundant checking.
**Expected behavior**
Iād like to eliminate this unsoundness by using stronger type system, which means create a new type for precision and scale:
```
struct DecimalInfo{
precision: usize,
scale: usize,
}
impl DecimalInfo{
fn new(precision: usize, scale: usize) -> Self {
assert (precision <= max_precision);
assert (scale <= max_precision);
assert (scale <= precision);
Self {precision, scale}
}
fn get_precision {...}
fn get_scale {...}
fn set_scale {...}
fn set_precision {...}
}
enum DataType {
Decimal128(DecimalInfo),
...
}
```
**Additional context**
<!--
Add any other context about the problem here.
-->
--
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.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] HaoYang670 commented on issue #2362: The constructor `DataType::Decimal(usize, usize)` is unsound.
Posted by GitBox <gi...@apache.org>.
HaoYang670 commented on issue #2362:
URL: https://github.com/apache/arrow-rs/issues/2362#issuecomment-1208102802
cc @alamb @viirya @tustvold @liukun4515
--
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 issue #2362: The constructor `DataType::Decimal(usize, usize)` is unsound.
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on issue #2362:
URL: https://github.com/apache/arrow-rs/issues/2362#issuecomment-1208178331
I want to go through the code from c++ or java version, and comment this issue.
--
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] viirya commented on issue #2362: The constructor `DataType::Decimal(usize, usize)` is unsound.
Posted by GitBox <gi...@apache.org>.
viirya commented on issue #2362:
URL: https://github.com/apache/arrow-rs/issues/2362#issuecomment-1208728543
I agree that the current decimal types constructors have the mentioned issue. It can be used to construct invalid decimal types.
My point above is, we have certain checks for it during performing value validation, in the places we want to make sure the data is valid (safe paths, š ). So it will be caught nowadays.
For the unsafe paths, we have many ways to mess the inputs, not just with the type constructor. So I don't think it is an issue there. Users should know what they're doing with such APIs.
The only holes, I guess, are where we don't do validation so invalid decimal type is not caught. But we also don't want to run real decimal value validation if we think it is computation heavy.
Alternatively, we can do only precision check on such places, instead of full value validation.
--
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 issue #2362: The constructor `DataType::Decimal(usize, usize)` is unsound.
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on issue #2362:
URL: https://github.com/apache/arrow-rs/issues/2362#issuecomment-1208179522
> I want to go through the code from c++ or java version, and comment this issue.
Because this changes will cause some api 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] viirya commented on issue #2362: The constructor `DataType::Decimal(usize, usize)` is unsound.
Posted by GitBox <gi...@apache.org>.
viirya commented on issue #2362:
URL: https://github.com/apache/arrow-rs/issues/2362#issuecomment-1208690415
> Here the problem is that we may do redundant validation.
For unsafe paths, I think we always do validation, no? Not sure if we can remove validation on unsafe paths. That is why they are "unsafe".
>
> That's the concern. `safe` means soundness. However `DataType::Decimal(usize, usize)` is unsound because it allows you to do `False` things without compile (or runtime) error. For example `Decimal(10000000000, 10000000000)` is invalid but you can do this.
I think that you can also do similar thing with safe paths, not only for this decimal type constructor. As we don't validate (not just for decimal arrays) values in safe paths, you can put some unreasonable input there and cause unexpected result. As it is safe paths, I think that the responsibility is on users hands.
--
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] viirya commented on issue #2362: The constructor `DataType::Decimal(usize, usize)` is unsound.
Posted by GitBox <gi...@apache.org>.
viirya commented on issue #2362:
URL: https://github.com/apache/arrow-rs/issues/2362#issuecomment-1208508623
C++ Decimal type has such check, e.g. Decimal128Type: https://github.com/apache/arrow/blob/6cc37cf2d1ba72c46b64fbc7ac499bd0d7296d20/cpp/src/arrow/type.cc#L870-L874
But I am not sure if we need to have the check. For unsafe paths, we run value validation which includes such check. For safe paths, I think it assumes the inputs are all valid (including decimal type). You can mess the array with either wrong type or input in safe paths.
It appears to me that's a bit verbose when constructing a decimal type with a struct or matching a decimal type. So I personally feel that it has a little advantage but also has some more disadvantage.
I don't feel strong against it, though.
--
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] HaoYang670 commented on issue #2362: The constructor `DataType::Decimal(usize, usize)` is unsound.
Posted by GitBox <gi...@apache.org>.
HaoYang670 commented on issue #2362:
URL: https://github.com/apache/arrow-rs/issues/2362#issuecomment-1208783641
Ok. I will file a PR and then we can have more discussion.
--
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] HaoYang670 commented on issue #2362: The constructor `DataType::Decimal(usize, usize)` is unsound.
Posted by GitBox <gi...@apache.org>.
HaoYang670 commented on issue #2362:
URL: https://github.com/apache/arrow-rs/issues/2362#issuecomment-1208686700
> For unsafe paths, we run value validation which includes such check.
Here the problem is that we may do redundant validation.
> For safe paths, I think it assumes the inputs are all valid (including decimal type).
That's the concern. `safe` means soundness. However `DataType::Decimal(usize, usize)` is unsound because it allows you to do `False` things without compile (or runtime) error. For example `Decimal(10000000000, 10000000000)` is invalid but you can do this.
--
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] alamb commented on issue #2362: The constructor `DataType::Decimal(usize, usize)` is unsound.
Posted by GitBox <gi...@apache.org>.
alamb commented on issue #2362:
URL: https://github.com/apache/arrow-rs/issues/2362#issuecomment-1209203156
> My point above is, we have certain checks for it during performing value validation, in the places we want to make sure the data is valid (safe paths, š ). So it will be caught nowadays.
I agree with @viirya
I think perhaps we were using different definitions of "unsound" . I was erroneously thinking it meant "unsafe" in the classic rust sense that doing so would allow reading/writing uninitialized memory, reading/writing out of bounds of the allocated memory, or data races.
It *doesn't mean* that user's can't provide bad inputs that result in `Err` or `panic!`, though I think many rust libraries try and do thorough error checking and uphold strong invariants
I think it is a good idea to make the arrow library be better about error checking in general as long as it isn't too cumbersome to use. Thank you @HaoYang670 for starting this conversation
--
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] HaoYang670 commented on issue #2362: The constructor `DataType::Decimal(usize, usize)` is unsound.
Posted by GitBox <gi...@apache.org>.
HaoYang670 commented on issue #2362:
URL: https://github.com/apache/arrow-rs/issues/2362#issuecomment-1208710360
> As it is safe paths, I think that the responsibility is on users hands.
Sorry, I don't 100% agree with you.
For safe (soundness) path, the responsibility in on the software side. Software should make sure all things it allows users to do is valid, otherwise, it is unsound.
For unsafe (completeness) path, the responsibility is on users hands, however. That is why we often see code like this in our crate:
```rust
// Soundness: users should make sure ...
unsafe {fn xxx_unchecked()}
```
--
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] HaoYang670 commented on issue #2362: The constructor `DataType::Decimal(usize, usize)` is unsound.
Posted by GitBox <gi...@apache.org>.
HaoYang670 commented on issue #2362:
URL: https://github.com/apache/arrow-rs/issues/2362#issuecomment-1208712965
I guess @tustvold has more concern on the soundness of the code.
--
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] viirya commented on issue #2362: The constructor `DataType::Decimal(usize, usize)` is unsound.
Posted by GitBox <gi...@apache.org>.
viirya commented on issue #2362:
URL: https://github.com/apache/arrow-rs/issues/2362#issuecomment-1208718799
Ah, yea, that's right. We did validation on safe paths, no unsafe paths...I thought "safe" and "unsafe" reversely. Please swap "safe" and "unsafe" in my previous comments.
(Updated)
> But I am not sure if we need to have the check. For safe paths, we run value validation which includes such check. For unsafe paths, I think it assumes the inputs are all valid (including decimal type). You can mess the array with either wrong type or input in unsafe paths.
--
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] HaoYang670 commented on issue #2362: The constructor `DataType::Decimal(usize, usize)` is unsound.
Posted by GitBox <gi...@apache.org>.
HaoYang670 commented on issue #2362:
URL: https://github.com/apache/arrow-rs/issues/2362#issuecomment-1208720382
Ha, we reach a consensus! š
That's why I say `DataType::Decimal(usize, usize)` is unsound.
--
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] viirya commented on issue #2362: The constructor `DataType::Decimal(usize, usize)` is unsound.
Posted by GitBox <gi...@apache.org>.
viirya commented on issue #2362:
URL: https://github.com/apache/arrow-rs/issues/2362#issuecomment-1208748054
> I don't feel strong against it, though.
BTW, as mentioned above, I'm okay for the proposed change if most of you think it is good to go.
--
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