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/04/15 12:03:40 UTC

[GitHub] [arrow] ritchie46 opened a new pull request #10046: ARROW-12398: Rust remove redundant bound check in iterators

ritchie46 opened a new pull request #10046:
URL: https://github.com/apache/arrow/pull/10046


   This PR removes the bound checks as discussed in #9994. 
   
   Furthermore I added `unsafe` versions of the `value` method to `PrimitiveArray` and `BooleanArray`. The `safe` marked methods are actually `unsafe`. This way we can slowly transition to explicitly using the `unsafe` variant and later make the "safe" one truly safe. 
   
   For the time being I also added a `debug_assert` bounds check in those "safe" methods that are `unsafe`. That way we at least get a panic in debug mode instead of UB in safe 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.

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

[GitHub] [arrow] tyrelr commented on a change in pull request #10046: ARROW-12398: [Rust] remove redundant bound check in iterators

Posted by GitBox <gi...@apache.org>.
tyrelr commented on a change in pull request #10046:
URL: https://github.com/apache/arrow/pull/10046#discussion_r615255644



##########
File path: rust/arrow/src/array/iterator.rs
##########
@@ -56,7 +56,7 @@ impl<'a, T: ArrowPrimitiveType> std::iter::Iterator for PrimitiveIter<'a, T> {
         } else {
             let old = self.current;
             self.current += 1;
-            Some(Some(self.array.value(old)))
+            unsafe { Some(Some(self.array.value_unchecked(old))) }

Review comment:
       Missing safety comment

##########
File path: rust/arrow/src/array/iterator.rs
##########
@@ -118,7 +118,9 @@ impl<'a> std::iter::Iterator for BooleanIter<'a> {
         } else {
             let old = self.current;
             self.current += 1;
-            Some(Some(self.array.value(old)))
+            // Safety:
+            // we just checked bounds

Review comment:
       I've been wanting to change this, thanks for beating me to it :)
   
   It's not critical, but could this clarify that the bounds check happens in the self.current_end comparison?  It seems to me this also depends on an undocumented struct invariant that self.current_end <= array.len().  Upon reading the comment, I had assumed that is_null was expected to be the bounds check, but ArrayData.is_null short circuits to false if there is no null buffer, so wouldn't always bounds check.  (this also applies to the other safety comments)

##########
File path: rust/arrow/src/array/iterator.rs
##########
@@ -77,7 +77,7 @@ impl<'a, T: ArrowPrimitiveType> std::iter::DoubleEndedIterator for PrimitiveIter
             Some(if self.array.is_null(self.current_end) {
                 None
             } else {
-                Some(self.array.value(self.current_end))
+                unsafe { Some(self.array.value_unchecked(self.current_end)) }

Review comment:
       Missing safety comment




-- 
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:
users@infra.apache.org

[GitHub] [arrow] ritchie46 commented on a change in pull request #10046: ARROW-12398: [Rust] remove redundant bound check in iterators

Posted by GitBox <gi...@apache.org>.
ritchie46 commented on a change in pull request #10046:
URL: https://github.com/apache/arrow/pull/10046#discussion_r615276337



##########
File path: rust/arrow/src/array/iterator.rs
##########
@@ -118,7 +118,9 @@ impl<'a> std::iter::Iterator for BooleanIter<'a> {
         } else {
             let old = self.current;
             self.current += 1;
-            Some(Some(self.array.value(old)))
+            // Safety:
+            // we just checked bounds

Review comment:
       Right, I will add that to prevent future confusion. :+1

##########
File path: rust/arrow/src/array/iterator.rs
##########
@@ -118,7 +118,9 @@ impl<'a> std::iter::Iterator for BooleanIter<'a> {
         } else {
             let old = self.current;
             self.current += 1;
-            Some(Some(self.array.value(old)))
+            // Safety:
+            // we just checked bounds

Review comment:
       Right, I will add that to prevent future confusion. :+1: 




-- 
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:
users@infra.apache.org

[GitHub] [arrow] codecov-io commented on pull request #10046: ARROW-12398: Rust remove redundant bound check in iterators

Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #10046:
URL: https://github.com/apache/arrow/pull/10046#issuecomment-820382389


   # [Codecov](https://codecov.io/gh/apache/arrow/pull/10046?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#10046](https://codecov.io/gh/apache/arrow/pull/10046?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c8b6908) into [master](https://codecov.io/gh/apache/arrow/commit/26a5a049b46c6949eb99f09a41da56232f0564c8?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (26a5a04) will **increase** coverage by `0.00%`.
   > The diff coverage is `100.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/arrow/pull/10046/graphs/tree.svg?width=650&height=150&src=pr&token=LpTCFbqVT1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/arrow/pull/10046?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@           Coverage Diff           @@
   ##           master   #10046   +/-   ##
   =======================================
     Coverage   78.70%   78.70%           
   =======================================
     Files         286      286           
     Lines       64213    64221    +8     
   =======================================
   + Hits        50538    50546    +8     
     Misses      13675    13675           
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/arrow/pull/10046?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [rust/arrow/src/array/array\_boolean.rs](https://codecov.io/gh/apache/arrow/pull/10046/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cnVzdC9hcnJvdy9zcmMvYXJyYXkvYXJyYXlfYm9vbGVhbi5ycw==) | `90.98% <100.00%> (+0.30%)` | :arrow_up: |
   | [rust/arrow/src/array/array\_primitive.rs](https://codecov.io/gh/apache/arrow/pull/10046/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cnVzdC9hcnJvdy9zcmMvYXJyYXkvYXJyYXlfcHJpbWl0aXZlLnJz) | `94.71% <100.00%> (+0.04%)` | :arrow_up: |
   | [rust/arrow/src/array/iterator.rs](https://codecov.io/gh/apache/arrow/pull/10046/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cnVzdC9hcnJvdy9zcmMvYXJyYXkvaXRlcmF0b3IucnM=) | `95.80% <100.00%> (ø)` | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow/pull/10046?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/arrow/pull/10046?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [c394a20...c8b6908](https://codecov.io/gh/apache/arrow/pull/10046?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
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:
users@infra.apache.org

[GitHub] [arrow] codecov-commenter commented on pull request #10046: ARROW-12398: [Rust] remove redundant bound check in iterators

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #10046:
URL: https://github.com/apache/arrow/pull/10046#issuecomment-821854603


   # [Codecov](https://codecov.io/gh/apache/arrow/pull/10046?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#10046](https://codecov.io/gh/apache/arrow/pull/10046?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d3819d0) into [master](https://codecov.io/gh/apache/arrow/commit/26a5a049b46c6949eb99f09a41da56232f0564c8?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (26a5a04) will **increase** coverage by `0.20%`.
   > The diff coverage is `95.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/arrow/pull/10046/graphs/tree.svg?width=650&height=150&src=pr&token=LpTCFbqVT1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/arrow/pull/10046?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #10046      +/-   ##
   ==========================================
   + Coverage   78.70%   78.91%   +0.20%     
   ==========================================
     Files         286      286              
     Lines       64213    64723     +510     
   ==========================================
   + Hits        50538    51075     +537     
   + Misses      13675    13648      -27     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/arrow/pull/10046?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [rust/arrow/src/array/iterator.rs](https://codecov.io/gh/apache/arrow/pull/10046/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cnVzdC9hcnJvdy9zcmMvYXJyYXkvaXRlcmF0b3IucnM=) | `95.80% <90.00%> (ø)` | |
   | [rust/arrow/src/array/array\_boolean.rs](https://codecov.io/gh/apache/arrow/pull/10046/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cnVzdC9hcnJvdy9zcmMvYXJyYXkvYXJyYXlfYm9vbGVhbi5ycw==) | `90.90% <100.00%> (+0.23%)` | :arrow_up: |
   | [rust/arrow/src/array/array\_primitive.rs](https://codecov.io/gh/apache/arrow/pull/10046/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cnVzdC9hcnJvdy9zcmMvYXJyYXkvYXJyYXlfcHJpbWl0aXZlLnJz) | `94.70% <100.00%> (+0.03%)` | :arrow_up: |
   | [...t/datafusion/src/physical\_optimizer/repartition.rs](https://codecov.io/gh/apache/arrow/pull/10046/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9waHlzaWNhbF9vcHRpbWl6ZXIvcmVwYXJ0aXRpb24ucnM=) | `91.80% <0.00%> (-4.92%)` | :arrow_down: |
   | [rust/datafusion/src/physical\_plan/mod.rs](https://codecov.io/gh/apache/arrow/pull/10046/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9waHlzaWNhbF9wbGFuL21vZC5ycw==) | `86.20% <0.00%> (-1.80%)` | :arrow_down: |
   | [rust/arrow/src/array/array.rs](https://codecov.io/gh/apache/arrow/pull/10046/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cnVzdC9hcnJvdy9zcmMvYXJyYXkvYXJyYXkucnM=) | `76.80% <0.00%> (-0.80%)` | :arrow_down: |
   | [rust/datafusion/src/scalar.rs](https://codecov.io/gh/apache/arrow/pull/10046/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9zY2FsYXIucnM=) | `49.38% <0.00%> (-0.74%)` | :arrow_down: |
   | [...ust/datafusion/src/physical\_plan/hash\_aggregate.rs](https://codecov.io/gh/apache/arrow/pull/10046/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9waHlzaWNhbF9wbGFuL2hhc2hfYWdncmVnYXRlLnJz) | `84.31% <0.00%> (-0.04%)` | :arrow_down: |
   | [rust/ballista/rust/scheduler/src/lib.rs](https://codecov.io/gh/apache/arrow/pull/10046/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cnVzdC9iYWxsaXN0YS9ydXN0L3NjaGVkdWxlci9zcmMvbGliLnJz) | `0.00% <0.00%> (ø)` | |
   | [rust/ballista/rust/client/src/context.rs](https://codecov.io/gh/apache/arrow/pull/10046/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cnVzdC9iYWxsaXN0YS9ydXN0L2NsaWVudC9zcmMvY29udGV4dC5ycw==) | `0.00% <0.00%> (ø)` | |
   | ... and [18 more](https://codecov.io/gh/apache/arrow/pull/10046/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow/pull/10046?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/arrow/pull/10046?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [c394a20...d3819d0](https://codecov.io/gh/apache/arrow/pull/10046?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
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:
users@infra.apache.org

[GitHub] [arrow] alamb closed pull request #10046: ARROW-12398: [Rust] remove redundant bound check in iterators

Posted by GitBox <gi...@apache.org>.
alamb closed pull request #10046:
URL: https://github.com/apache/arrow/pull/10046


   


-- 
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:
users@infra.apache.org

[GitHub] [arrow] Dandandan commented on pull request #10046: ARROW-12398: Rust remove redundant bound check in iterators

Posted by GitBox <gi...@apache.org>.
Dandandan commented on pull request #10046:
URL: https://github.com/apache/arrow/pull/10046#issuecomment-820485697


   > This PR removes the bound checks as discussed in #9994.
   > 
   > Furthermore I added `unsafe` versions of the `value` method to `PrimitiveArray` and `BooleanArray`. The `safe` marked methods are actually `unsafe`. This way we can slowly transition to explicitly using the `unsafe` variant and later make the "safe" one truly safe.
   > 
   > For the time being I also added a `debug_assert` bounds check in those "safe" methods that are `unsafe`. That way we at least get a panic in debug mode instead of UB in safe code.
   
   This is great! I think this is a great improvement from the `unsafe` standpoint, and this helps us limit the unsafe method only in places where it's needed (iterators/hot loops)!


-- 
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:
users@infra.apache.org

[GitHub] [arrow] ritchie46 commented on pull request #10046: ARROW-12398: [Rust] remove redundant bound check in iterators

Posted by GitBox <gi...@apache.org>.
ritchie46 commented on pull request #10046:
URL: https://github.com/apache/arrow/pull/10046#issuecomment-821857950


   Ok, I added some extra safety comments. I think its good to go now.
   
   I believe the failures in [CI are related to this](https://github.com/apache/arrow/pull/10081)


-- 
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:
users@infra.apache.org

[GitHub] [arrow] alamb commented on pull request #10046: ARROW-12398: [Rust] remove redundant bound check in iterators

Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #10046:
URL: https://github.com/apache/arrow/pull/10046#issuecomment-821971670


   I tested this branch locally with `cargo +nightly-2021-03-24 test --features "simd"` (aka picking up the changes from https://github.com/apache/arrow/pull/10082) and it passed for me. So merging this one in


-- 
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:
users@infra.apache.org

[GitHub] [arrow] Dandandan commented on a change in pull request #10046: ARROW-12398: Rust remove redundant bound check in iterators

Posted by GitBox <gi...@apache.org>.
Dandandan commented on a change in pull request #10046:
URL: https://github.com/apache/arrow/pull/10046#discussion_r614133898



##########
File path: rust/arrow/src/array/array_primitive.rs
##########
@@ -88,12 +88,23 @@ impl<T: ArrowPrimitiveType> PrimitiveArray<T> {
         PrimitiveBuilder::<T>::new(capacity)
     }
 
+    /// Returns the primitive value at index `i`.
+    ///
+    /// # Safety
+    ///
+    /// caller must ensure that the passed in offset is less than the array len()
+    pub unsafe fn value_unchecked(&self, i: usize) -> T::Native {
+        let offset = i + self.offset();
+        *self.raw_values.as_ptr().add(offset)
+    }
+
     /// Returns the primitive value at index `i`.
     ///
     /// Note this doesn't do any bound checking, for performance reason.
     /// # Safety
     /// caller must ensure that the passed in offset is less than the array len()
     pub fn value(&self, i: usize) -> T::Native {
+        debug_assert!(i < self.len());
         let offset = i + self.offset();

Review comment:
       This might use `value_unchecked` now?




-- 
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:
users@infra.apache.org

[GitHub] [arrow] github-actions[bot] commented on pull request #10046: ARROW-12398: Rust remove redundant bound check in iterators

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #10046:
URL: https://github.com/apache/arrow/pull/10046#issuecomment-820371284


   https://issues.apache.org/jira/browse/ARROW-12398


-- 
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:
users@infra.apache.org