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/06/08 10:36:46 UTC
[GitHub] [arrow-rs] tustvold opened a new pull request, #1819: Seal ArrowNativeType (#1028)
tustvold opened a new pull request, #1819:
URL: https://github.com/apache/arrow-rs/pull/1819
# Which issue does this PR close?
Closes #1028
# Rationale for this change
`ArrowNativeType` is used to indicate "trivially safely transmutable" within the buffer implementations. If client can create their own implementations they can do potentially unwise things.
# What changes are included in this PR?
Makes `ArrowNativeType` sealed
# Are there any user-facing changes?
Technically yes, practically I can't think of any valid use-cases this will break.
--
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 a diff in pull request #1819: Seal ArrowNativeType and OffsetSizeTrait (#1028)
Posted by GitBox <gi...@apache.org>.
jhorstmann commented on code in PR #1819:
URL: https://github.com/apache/arrow-rs/pull/1819#discussion_r892557312
##########
arrow/src/datatypes/native.rs:
##########
@@ -351,8 +382,11 @@ impl JsonSerializable for f64 {
}
impl ArrowNativeType for f16 {}
+impl private::Sealed for f16 {}
impl ArrowNativeType for f32 {}
+impl private::Sealed for f32 {}
impl ArrowNativeType for f64 {}
+impl private::Sealed for f64 {}
/// Allows conversion from supported Arrow types to a byte slice.
pub trait ToByteSlice {
Review Comment:
Right, I did not word that right. The question is rather whether we want to support other (external) types to be used for example in `MutableBuffer`:
```
pub fn push<T: ToByteSlice>(&mut self, item: T)
```
It's probably fine as it would only be unsound if the trait impl does something explicitly 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] tustvold commented on a diff in pull request #1819: Seal ArrowNativeType and OffsetSizeTrait (#1028)
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #1819:
URL: https://github.com/apache/arrow-rs/pull/1819#discussion_r892547691
##########
arrow/src/datatypes/native.rs:
##########
@@ -351,8 +382,11 @@ impl JsonSerializable for f64 {
}
impl ArrowNativeType for f16 {}
+impl private::Sealed for f16 {}
impl ArrowNativeType for f32 {}
+impl private::Sealed for f32 {}
impl ArrowNativeType for f64 {}
+impl private::Sealed for f64 {}
/// Allows conversion from supported Arrow types to a byte slice.
pub trait ToByteSlice {
Review Comment:
I don't think ToByteSlice assumes transmutability as far as I can tell, although the blanket impl for ArrowNativeType is (which is fine)? Am I missing something?
```
pub trait ToByteSlice {
/// Converts this instance into a byte slice
fn to_byte_slice(&self) -> &[u8];
}
impl<T: ArrowNativeType> ToByteSlice for [T] {
#[inline]
fn to_byte_slice(&self) -> &[u8] {
let raw_ptr = self.as_ptr() as *const T as *const u8;
unsafe {
std::slice::from_raw_parts(raw_ptr, self.len() * std::mem::size_of::<T>())
}
}
}
impl<T: ArrowNativeType> ToByteSlice for T {
#[inline]
fn to_byte_slice(&self) -> &[u8] {
let raw_ptr = self as *const T as *const u8;
unsafe { std::slice::from_raw_parts(raw_ptr, std::mem::size_of::<T>()) }
}
}
```
--
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 #1819: Seal ArrowNativeType and OffsetSizeTrait (#1028)
Posted by GitBox <gi...@apache.org>.
tustvold commented on PR #1819:
URL: https://github.com/apache/arrow-rs/pull/1819#issuecomment-1149836238
Good point, this will transitively also seal `OffsetSizeTrait`
--
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 a diff in pull request #1819: Seal ArrowNativeType and OffsetSizeTrait (#1028)
Posted by GitBox <gi...@apache.org>.
jhorstmann commented on code in PR #1819:
URL: https://github.com/apache/arrow-rs/pull/1819#discussion_r892529479
##########
arrow/src/datatypes/native.rs:
##########
@@ -351,8 +382,11 @@ impl JsonSerializable for f64 {
}
impl ArrowNativeType for f16 {}
+impl private::Sealed for f16 {}
impl ArrowNativeType for f32 {}
+impl private::Sealed for f32 {}
impl ArrowNativeType for f64 {}
+impl private::Sealed for f64 {}
/// Allows conversion from supported Arrow types to a byte slice.
pub trait ToByteSlice {
Review Comment:
The `ToByteSlice` trait would also be a candiate for sealing as it also used for transmuting and should only be used on ArrowNativeTypes and slices of ArrowNativeType.
--
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 #1819: Seal ArrowNativeType and OffsetSizeTrait (#1028)
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #1819:
URL: https://github.com/apache/arrow-rs/pull/1819#discussion_r892498873
##########
arrow/src/datatypes/native.rs:
##########
@@ -19,6 +19,10 @@ use super::DataType;
use half::f16;
use serde_json::{Number, Value};
+mod private {
+ pub trait Sealed {}
Review Comment:
Added in https://github.com/apache/arrow-rs/pull/1819/commits/a64575e5bb642035ff3c3a17d3e003af7416269c
--
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 pull request #1819: Seal ArrowNativeType and OffsetSizeTrait (#1028)
Posted by GitBox <gi...@apache.org>.
HaoYang670 commented on PR #1819:
URL: https://github.com/apache/arrow-rs/pull/1819#issuecomment-1149962958
> This PR's description says "seal OffsetSizeTrait but I didn't see any change to that trait
@alamb I guess we don't need to change `OffsetSizeTrait` because `ArrowNativeType` is a super trait of `OffsetSizeTrait`. If `ArrowNativeType` is sealed, `OffsetSizeTrait` will be automatically sealed. (not verified 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] alamb commented on a diff in pull request #1819: Seal ArrowNativeType and OffsetSizeTrait (#1028)
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #1819:
URL: https://github.com/apache/arrow-rs/pull/1819#discussion_r892360650
##########
arrow/src/datatypes/native.rs:
##########
@@ -19,6 +19,10 @@ use super::DataType;
use half::f16;
use serde_json::{Number, Value};
+mod private {
+ pub trait Sealed {}
Review Comment:
I think we should add some docstrings about *why* it is not allowed to ArrowNativeType
Using the text from the PR description "ArrowNativeType is used to indicate "trivially safely transmutable" within the buffer implementations. If client can create their own implementations they can do potentially unwise things."
would be a good starting point
--
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 #1819: Seal ArrowNativeType and OffsetSizeTrait (#1028)
Posted by GitBox <gi...@apache.org>.
tustvold commented on PR #1819:
URL: https://github.com/apache/arrow-rs/pull/1819#issuecomment-1149990671
> This PR's description says "seal OffsetSizeTrait but I didn't see any change to that trait in
* ArrowNativeType is sealed and so cannot be implemented for types outside of arrow-rs
* OffsetSizeTrait can only be implemented for ArrowNativeType
* Trait orphan rules prevent implementing OffsetSizeTrait for foreign types outside of arrow-rs
These three things together seal OffsetSizeTrait
--
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 pull request #1819: Seal ArrowNativeType and OffsetSizeTrait (#1028)
Posted by GitBox <gi...@apache.org>.
HaoYang670 commented on PR #1819:
URL: https://github.com/apache/arrow-rs/pull/1819#issuecomment-1150502426
> * ArrowNativeType is sealed and so cannot be implemented for types outside of arrow-rs
> * OffsetSizeTrait can only be implemented for ArrowNativeType
> * Trait orphan rules prevent implementing OffsetSizeTrait for foreign types outside of arrow-rs
>
> These three things together seal OffsetSizeTrait
Sorry, I find that we should further seal the `OffsetSizeTrait` because `Offset` only support `i32` and `i64`. I will file a followup PR.
--
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 a diff in pull request #1819: Seal ArrowNativeType and OffsetSizeTrait (#1028)
Posted by GitBox <gi...@apache.org>.
HaoYang670 commented on code in PR #1819:
URL: https://github.com/apache/arrow-rs/pull/1819#discussion_r892284313
##########
arrow/src/datatypes/native.rs:
##########
@@ -19,6 +19,10 @@ use super::DataType;
use half::f16;
use serde_json::{Number, Value};
+mod private {
+ pub trait Sealed {}
Review Comment:
We could implement the `Seal` trait in the `private` mod:
```rust
mod private {
pub trait Sealed {}
impl Sealed for i8 {}
impl Sealed for i16 {}
}
```
--
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 pull request #1819: Seal ArrowNativeType (#1028)
Posted by GitBox <gi...@apache.org>.
HaoYang670 commented on PR #1819:
URL: https://github.com/apache/arrow-rs/pull/1819#issuecomment-1149835134
Great! This could also seal the `OffsetSizeTrait`!
--
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 #1819: Seal ArrowNativeType and OffsetSizeTrait (#1028)
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #1819:
URL: https://github.com/apache/arrow-rs/pull/1819#discussion_r892459671
##########
arrow/src/datatypes/native.rs:
##########
@@ -19,6 +19,10 @@ use super::DataType;
use half::f16;
use serde_json::{Number, Value};
+mod private {
+ pub trait Sealed {}
Review Comment:
I think I prefer implementing it with the rest of the implementations for the type. It makes it clearer why it is implemented, and theoretically we may macroify it later
--
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 #1819: Seal ArrowNativeType and OffsetSizeTrait (#1028)
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #1819:
URL: https://github.com/apache/arrow-rs/pull/1819#discussion_r892547691
##########
arrow/src/datatypes/native.rs:
##########
@@ -351,8 +382,11 @@ impl JsonSerializable for f64 {
}
impl ArrowNativeType for f16 {}
+impl private::Sealed for f16 {}
impl ArrowNativeType for f32 {}
+impl private::Sealed for f32 {}
impl ArrowNativeType for f64 {}
+impl private::Sealed for f64 {}
/// Allows conversion from supported Arrow types to a byte slice.
pub trait ToByteSlice {
Review Comment:
I don't think ToByteSlice assumes transmutability as far as I can tell, although the blanket impl for ArrowNativeType is (which is fine)? Are the
```
pub trait ToByteSlice {
/// Converts this instance into a byte slice
fn to_byte_slice(&self) -> &[u8];
}
impl<T: ArrowNativeType> ToByteSlice for [T] {
#[inline]
fn to_byte_slice(&self) -> &[u8] {
let raw_ptr = self.as_ptr() as *const T as *const u8;
unsafe {
std::slice::from_raw_parts(raw_ptr, self.len() * std::mem::size_of::<T>())
}
}
}
impl<T: ArrowNativeType> ToByteSlice for T {
#[inline]
fn to_byte_slice(&self) -> &[u8] {
let raw_ptr = self as *const T as *const u8;
unsafe { std::slice::from_raw_parts(raw_ptr, std::mem::size_of::<T>()) }
}
}
```
--
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 #1819: Seal ArrowNativeType and OffsetSizeTrait (#1028)
Posted by GitBox <gi...@apache.org>.
tustvold merged PR #1819:
URL: https://github.com/apache/arrow-rs/pull/1819
--
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