[GitHub] [arrow] alamb commented on pull request #8007: ARROW-9790: [Rust][Parquet] Fix PrimitiveArrayReader boundary conditions

[GitBox <gi...@apache.org>]

alamb commented on pull request #8007:
URL: https://github.com/apache/arrow/pull/8007#issuecomment-676450197


   As the code that I removed in this PR  added to support empty pages in 8a61570a8bd64ae6f6e47ce8efd2287c3c2feded (but it didn't seem to have any tests), I was worried that this deletion would cause some sort of reversion in behavior. To ensure it didn't, I added a test for empty pages in commit cfdea8349a8f41229a4c8fb2cfddec3bb11df112
   
   To try and ensure that the test covers the empty pages case, I and ran the new test on commit ed1f771dccdde623ce85e212eccb2b573185c461 (the one right before 8a61570a8bd64ae6f6e47ce8efd2287c3c2feded) and the test fails, thus leading me to conclude that the code removed in this PR is unnecessary and zero page parquet files are still supported.
   
   Log of what I did:
   
   Get commit right before support for empty pages:
   ```
   alamb@MacBook-Pro rust % git checkout ed1f771dccdde623ce85e212eccb2b573185c461
   git checkout ed1f771dccdde623ce85e212eccb2b573185c461
   M	testing
   Note: switching to 'ed1f771dccdde623ce85e212eccb2b573185c461'.
   ...
   HEAD is now at ed1f771dc ARROW-8717: [CI][Packaging] Add build dependency on boost to homebrew
   ```
   
   Apply new test for no pages:
   
   ```
   alamb@MacBook-Pro rust % git cherry-pick cfdea8349a8f41229a4c8fb2cfddec3bb11df112
   git cherry-pick cfdea8349a8f41229a4c8fb2cfddec3bb11df112
   Auto-merging rust/parquet/src/arrow/array_reader.rs
   [detached HEAD ab3bcdd8f] Add a test for reading empty pages
    Date: Wed Aug 19 10:16:31 2020 -0400
    1 file changed, 57 insertions(+), 1 deletion(-)
   ```
   
   Then I had to edit the test a little to use `.len` instead of `.is_empty` (which did not exist in the old commit):
   
   ```
   diff --git a/rust/parquet/src/arrow/array_reader.rs b/rust/parquet/src/arrow/array_reader.rs
   index d70619518..54c929c05 100644
   --- a/rust/parquet/src/arrow/array_reader.rs
   +++ b/rust/parquet/src/arrow/array_reader.rs
   @@ -967,7 +967,7 @@ mod tests {
   
            // expect no values to be read
            let array = array_reader.next_batch(50).unwrap();
   -        assert!(array.is_empty());
   +        assert!(array.len() == 0);
        }
   
        #[test]
   ```
   
   And then I ran the test, and it fails:
   
   ```
   cd arrow/rust/parquet && PARQUET_TEST_DATA=`pwd`/../../cpp/submodules/parquet-testing/data ARROW_TEST_DATA=`pwd`/../../testing/data cargo test
   ...
   ---- arrow::array_reader::tests::test_primitive_array_reader_empty_pages stdout ----
   thread 'arrow::array_reader::tests::test_primitive_array_reader_empty_pages' panicked at 'called `Result::unwrap()` on an `Err` value: General("Can\'t build array without pages!")', parquet/src/arrow/array_reader.rs:962:32
   note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
   ```
   


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