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/12 10:16:53 UTC

[GitHub] [arrow] ritchie46 opened a new pull request #9994: ARROW-12337: [Rust] add DoubleEndedIterator and ExactSizeIterator traits

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


   This PR implements the traits `DoubleEndedIterator` and `ExactSizeIterator` for the arrow array iterators. For the trait `ExactSizeIterator` this is an indication of the types system that their size is known, and `DoubleEndedIterator`  make them iterable in reverse order. Both include the improve of the iterators.
   
   Regarding this, I notice that the iterators check bounds twice. 
   
   ```rust
       fn next(&mut self) -> Option<Self::Item> {
           let i = self.current;
           if i >= self.current_end { // first bounds check
               None
           } else if self.array.is_null(i) {
               self.current += 1;
               Some(None)
           } else {
               self.current += 1;
               Some(Some(self.array.value(i)))  // second bounds check in `array.value`
           }
       }
   ```
   
   In some implementations `self.array.value` includes a second bounds check. Shall I propose a PR that uses `self.array.value_unchecked`. This is safe as the bounds are already checked.


-- 
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 #9994: ARROW-12337: [Rust] add DoubleEndedIterator and ExactSizeIterator traits

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


   


-- 
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 #9994: ARROW-12337: [Rust] add DoubleEndedIterator and ExactSizeIterator traits

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


   > This looks great -- thanks @ritchie46
   
   Great! :), than I will follow up with the bounds check 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.

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

[GitHub] [arrow] github-actions[bot] commented on pull request #9994: ARROW-12337: [Rust] add DoubleEndedIterator and ExactSizeIterator traits

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


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


-- 
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 a change in pull request #9994: ARROW-12337: [Rust] add DoubleEndedIterator and ExactSizeIterator traits

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



##########
File path: rust/arrow/src/array/iterator.rs
##########
@@ -155,17 +155,17 @@ where
     T: StringOffsetSizeTrait,
 {
     array: &'a GenericStringArray<T>,
-    i: usize,
-    len: usize,
+    current: usize,

Review comment:
       These are much better names 👍 




-- 
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 #9994: ARROW-12337: [Rust] add DoubleEndedIterator and ExactSizeIterator traits

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


   > The only thing I think this PR is missing are some tests (not really to show that this implementation is correct, which I think it is, but to ensure we don't accidentally break the functionality in some future refactoring)
   
   @alamb Good idea, I will do that tomorrow.


-- 
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