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 2021/12/03 15:25:04 UTC
[GitHub] [arrow-rs] alamb opened a new issue #996: Remove unsafe from Buffer::typed_data
alamb opened a new issue #996:
URL: https://github.com/apache/arrow-rs/issues/996
**Is your feature request related to a problem or challenge? Please describe what you are trying to do.**
As pointed out by @jhorstmann on #921 at https://github.com/apache/arrow-rs/pull/921#discussion_r760086317,
`Buffer::typed_data` does not actually need to be marked unsafe since it checks the alignment requirements. The very similarly implemented `MutableBuffer::typed_data_mut` is not marked as unsafe. The safety notes mention bool as a special case, but that is no longer an ArrowNativeType since a while.
https://github.com/apache/arrow-rs/blob/6a6e7f7/arrow/src/buffer/immutable.rs#L160-L181
**Describe the solution you'd like**
1. Remove `unsafe` from `Buffer::typed_data`
2. Remove note about bool in docstrings
--
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 #996: Remove unsafe from Buffer::typed_data
Posted by GitBox <gi...@apache.org>.
alamb commented on issue #996:
URL: https://github.com/apache/arrow-rs/issues/996#issuecomment-991636084
Follow on tickets:
* https://github.com/apache/arrow-rs/issues/1027
* https://github.com/apache/arrow-rs/issues/1028
--
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 #996: Remove unsafe from Buffer::typed_data
Posted by GitBox <gi...@apache.org>.
alamb commented on issue #996:
URL: https://github.com/apache/arrow-rs/issues/996#issuecomment-986042551
Closing this one as I don't think it is actionable upon further review. FYI @jhorstmann
--
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] jhorstmann commented on issue #996: Remove unsafe from Buffer::typed_data
Posted by GitBox <gi...@apache.org>.
jhorstmann commented on issue #996:
URL: https://github.com/apache/arrow-rs/issues/996#issuecomment-986046297
Ok, I wasn't aware that implementing ArrowNativeType for arbitrary types by a user is a supported usecase. Maybe making that into a sealed trait would also be an option. Anyway, my comment was more about the inconsistency between `MutableBuffer::typed_data` and `Buffer::typed_data`, so marking both `unsafe` is also fine.
--
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 #996: Remove unsafe from Buffer::typed_data
Posted by GitBox <gi...@apache.org>.
alamb commented on issue #996:
URL: https://github.com/apache/arrow-rs/issues/996#issuecomment-991636084
Follow on tickets:
* https://github.com/apache/arrow-rs/issues/1027
* https://github.com/apache/arrow-rs/issues/1028
--
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 #996: Remove unsafe from Buffer::typed_data
Posted by GitBox <gi...@apache.org>.
alamb commented on issue #996:
URL: https://github.com/apache/arrow-rs/issues/996#issuecomment-986042173
I reviewed the code a bit more, and the comments make a great point:
```
/// `ArrowNativeType` is public so that it can be used as a trait bound for other public
/// components, such as the `ToByteSlice` trait. However, this means that it can be
/// implemented by user defined types, which it is not intended for.
```
Meaning that if a user implements `ArrowNativeType` for their types, this will result in undefined behavior.
I also looked more carefully at Buffer and it effectively allows reinterpreting arbitrary `bytes` as different types, so I am not sure that behavior is safe...
What probably should be done is make `MutableBuffer::typed_data` unsafe instead as the docs there say very specifically
```
/// # Safety
/// This function must only be used when this buffer was extended with items of type `T`.
/// Failure to do so results in undefined behavior.
```
--
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 closed issue #996: Remove unsafe from Buffer::typed_data
Posted by GitBox <gi...@apache.org>.
alamb closed issue #996:
URL: https://github.com/apache/arrow-rs/issues/996
--
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 #996: Remove unsafe from Buffer::typed_data
Posted by GitBox <gi...@apache.org>.
alamb commented on issue #996:
URL: https://github.com/apache/arrow-rs/issues/996#issuecomment-986235011
Sounds good -- I'll file some follow on tickets.
On Sat, Dec 4, 2021 at 10:35 AM Jörn Horstmann ***@***.***>
wrote:
> Ok, I wasn't aware that implementing ArrowNativeType for arbitrary types
> by a user is a supported usecase. Maybe making that into a sealed trait
> would also be an option. Anyway, my comment was more about the
> inconsistency between MutableBuffer::typed_data and Buffer::typed_data,
> so marking both unsafe is also fine.
>
> —
> You are receiving this because you modified the open/close state.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/arrow-rs/issues/996#issuecomment-986046297>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AADXZMIHSUGILKQBP7XUE7DUPIYMPANCNFSM5JJ6HAWQ>
> .
> Triage notifications on the go with GitHub Mobile for iOS
> <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
> or Android
> <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
>
>
--
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 edited a comment on issue #996: Remove unsafe from Buffer::typed_data
Posted by GitBox <gi...@apache.org>.
alamb edited a comment on issue #996:
URL: https://github.com/apache/arrow-rs/issues/996#issuecomment-986042173
I reviewed the code a bit more, and the comments make a great point:
```
/// `ArrowNativeType` is public so that it can be used as a trait bound for other public
/// components, such as the `ToByteSlice` trait. However, this means that it can be
/// implemented by user defined types, which it is not intended for.
```
Meaning that if a user implements `ArrowNativeType` for their types, this will result in undefined behavior.
I also looked more carefully at Buffer and it effectively allows reinterpreting arbitrary `bytes` as different types, so I am not sure that behavior is safe...
What could
be done is make `MutableBuffer::typed_data` unsafe instead as the docs there say very specifically
```
/// # Safety
/// This function must only be used when this buffer was extended with items of type `T`.
/// Failure to do so results in undefined behavior.
```
--
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