Fix record delimiting on row group boundaries (#2025)

[tustvold]

Which issue does this PR close?

Closes #2025

Rationale for this change

The previous behaviour was incorrect

What changes are included in this PR?

Fixes the delimiting logic in RecordReader

Are there any user-facing changes?

No

[tustvold]

Drive by fix

[tustvold]

This is the cause of the bug, the end of the column chunk was being detected based on if read_on_batch returned less than the batch size. If the batch_size happens to exactly match the remaining records this would fail, and the reader would miss the last record in the chunk

[tustvold]

I don't understand what the CI is doing, it seems to be checking out some ref that doesn't exist???

[tustvold]

Turns out it was a combination of PEBCAC and a logical conflict when CI merged the lastest master in.

[alamb]

This test passes on master, without the changes in this PR 🤔

[tustvold]

This is somewhat expected, I'll see if I can make it fail

[alamb]

When I remove the code in this PR this test fails like the following (I was expecting the reader to miss the last record)

---- arrow::arrow_reader::tests::test_row_group_exact_multiple stdout ----
thread 'arrow::arrow_reader::tests::test_row_group_exact_multiple' panicked at 'called `Result::unwrap()` on an `Err` value: ParquetError("Parquet error: Not all children array length are the same!")', parquet/src/arrow/arrow_reader.rs:1581:53
stack backtrace:
   0: rust_begin_unwind
             at /rustc/a8314ef7d0ec7b75c336af2c9857bfaf43002bfc/library/std/src/panicking.rs:584:5
   1: core::panicking::panic_fmt
             at /rustc/a8314ef7d0ec7b75c336af2c9857bfaf43002bfc/library/core/src/panicking.rs:142:14
   2: core::result::unwrap_failed
             at /rustc/a8314ef7d0ec7b75c336af2c9857bfaf43002bfc/library/core/src/result.rs:1785:5
   3: core::result::Result<T,E>::unwrap
             at /rustc/a8314ef7d0ec7b75c336af2c9857bfaf43002bfc/library/core/src/result.rs:1078:23
   4: parquet::arrow::arrow_reader::tests::test_row_group_exact_multiple
             at ./src/arrow/arrow_reader.rs:1581:23
   5: parquet::arrow::arrow_reader::tests::test_row_group_exact_multiple::{{closure}}
             at ./src/arrow/arrow_reader.rs:1533:5
   6: core::ops::function::FnOnce::call_once
             at /rustc/a8314ef7d0ec7b75c336af2c9857bfaf43002bfc/library/core/src/ops/function.rs:248:5
   7: core::ops::function::FnOnce::call_once
             at /rustc/a8314ef7d0ec7b75c336af2c9857bfaf43002bfc/library/core/src/ops/function.rs:248:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

[tustvold]

It only misses the last record for the repeated array, and so you end up with this error

[alamb]

@tustvold lint failed !

[tustvold]

So the batch_size bug was a bug, but would be masked by the MIN_BATCH_SIZE setting of 1024 and so would only occur with batch sizes >= 1024. The actual cause of the failure was more subtle. The bug was that once exhausted RecordReader would continue to re-read the last record on subsequent calls to read_records. This would only occur if it returned exactly the batch_size number of records, when reaching the end of a chunk. This wouldn't actually read any new data and so would end up actually returning less records than it claimed to have read.

So read_records would do the following

• Read 8 records
• Return the corresponding values to the caller
• Read 1 phantom record
• Read 7 records
• Return this to the caller claiming it to be 8 records, but actually only being 7
• Read the remaining 5 records

If only a single list column this somewhat funky behaviour would not be noticeable, as it would eventually read all the data, corresponding to the total number of rows. However, it would read batches of 8, 7, 5 instead of 8, 8, 4, which would throw off the StructArrayReader.