[GitHub] [arrow] ericwburden commented on a change in pull request #9624: ARROW-11845: [Rust] Implement to_isize() for ArrowNativeTypes

2021-03-03 Thread GitBox 


ericwburden commented on a change in pull request #9624:
URL: https://github.com/apache/arrow/pull/9624#discussion_r586483501



##
File path: rust/arrow/src/array/array_binary.rs
##
@@ -89,7 +89,7 @@ impl 
GenericBinaryArray {
 // pointer alignment & location is ensured by RawPtrBox
 // buffer bounds/offset is ensured by the value_offset invariants
 std::slice::from_raw_parts(
-self.value_data.as_ptr().offset(start.to_isize()),
+self.value_data.as_ptr().offset(start.to_isize().unwrap()),

Review comment:
   I do think it would be good form to add the explanatory comments, 
though. I'll just hang out and see if anyone corrects my logic, then commit the 
comments.





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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [arrow] ericwburden commented on a change in pull request #9624: ARROW-11845: [Rust] Implement to_isize() for ArrowNativeTypes

2021-03-03 Thread GitBox 


ericwburden commented on a change in pull request #9624:
URL: https://github.com/apache/arrow/pull/9624#discussion_r586482079



##
File path: rust/arrow/src/array/array_binary.rs
##
@@ -89,7 +89,7 @@ impl 
GenericBinaryArray {
 // pointer alignment & location is ensured by RawPtrBox
 // buffer bounds/offset is ensured by the value_offset invariants
 std::slice::from_raw_parts(
-self.value_data.as_ptr().offset(start.to_isize()),
+self.value_data.as_ptr().offset(start.to_isize().unwrap()),

Review comment:
   Actually, now that I consider it, the implementation of `to_isize()` 
that I removed from OffsetSizeTrait already had the unwraps, just inside the 
`to_isize()` function instead of on the result of `to_isize()`. So, at the very 
least, we shouldn't be adding any risks we weren't already taking.





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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [arrow] ericwburden commented on a change in pull request #9624: ARROW-11845: [Rust] Implement to_isize() for ArrowNativeTypes

2021-03-03 Thread GitBox 


ericwburden commented on a change in pull request #9624:
URL: https://github.com/apache/arrow/pull/9624#discussion_r586473814



##
File path: rust/arrow/src/array/array_binary.rs
##
@@ -89,7 +89,7 @@ impl 
GenericBinaryArray {
 // pointer alignment & location is ensured by RawPtrBox
 // buffer bounds/offset is ensured by the value_offset invariants
 std::slice::from_raw_parts(
-self.value_data.as_ptr().offset(start.to_isize()),
+self.value_data.as_ptr().offset(start.to_isize().unwrap()),

Review comment:
   Here's my rationale:
   
   > `start` is an , which is a generic type that implements the 
   > OffsetSizeTrait. Currently, only i32 and i64 implement OffsetSizeTrait,
   > both of which should cleanly cast to isize on an architecture that supports
   > 32/64-bit offsets
   
   I *think* that is true. Can anyone think of a situation in which that would 
not hold? (For reference, I'm primarily an R programmer, so questions about 
memory address size are a bit arcane for me).





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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org