@sunchao @nevi-me @andygrove Please help to review this.

Also we should change the title of this PR: Implement Arrow record reader.

@sunchao Thanks for the review. Yes you are right, this is heavily inspired by c++ implementation. Currently c++ implementation can handle List<List<List> type, I'll extend this to handle more cases. Also I took a look at triple.rs. Though it's similar to record reader, they serve different purposes. The main goal of record reader is to help arrow array reader to find the boundaries of nested records and buffer them, while triple.rs is much simpler.

@liurenjie1024 Ouch.

The list collection is actually done in reader.rs. triplet.rs's goal is producing triplets of spaced values, accounting for definition levels. I am not familiar with arrow reader. Can c++ code handle List<List<List<List<List<...>>>>? It should not matter how many nested levels it has, the algorithm should just work (which might suggest that c++ code might be missing something).

I looked at the code, could you add more documentation around it, otherwise it is quite difficult to understand what is going on? Also would be good to add more tests around different complex schemas including deeply nested nullable lists and maps.

Thanks!

@sadikovi
Currently c++ code can handle any nested level of list, I just didn't express it clearly.
Just took a look at reader.rs. What record_reader.rs does for arrow_reader(which I'll send a PR later after this) is similar to what triplet.rs does for reader.rs. Seems like this name record_reader is misleading, it should be called something like record_splitter.
In fact I have tests for nullable lists, but not maps because currently arrow rust implementation doesn't support map. More nested lists may not help because the main logic is of splitting records are the same.

@sunchao @sadikovi The following code is from primitive array reader (WIP: read parquet column into arrow primitive array), it demonstrates usage of record reader.

impl<T: DataType> ArrayReader for PrimitiveArrayReader<T> {
    fn next_batch(&mut self, batch_size: usize) -> Result<Arc<Array>> {
        self.record_reader.reset()?;

        let mut records_read = 0usize;
        while records_read < batch_size {
            let records_to_read = batch_size - records_read;

            let records_read_once = self.record_reader.read_records(records_to_read)?;
            records_read = records_read + records_read_once;

            // Record reader exhausted
            if records_read_once < records_to_read {
                if let Some(page_reader) = self.pages.next() {
                    // Read from new page reader
                    self.record_reader.set_page_reader(page_reader?)?;
                } else {
                    // Page reader also exhausted
                    break;
                }
            }
        }

       // Convert buffer data in record_reader to array
       let record_data = self.record_reader.consume_record_data();
       let null_bitmap = self.consume_bitmap();
       .....
    }
}

Sorry for late review @liurenjie1024 !. I'll take another thorough look on this in the weekend.

@sunchao Thanks for the review.
It seems that it's a little difficult to understand how this works without a full picture of the implementation. I'll submit a PR for initial version of arrow reader later.

@sunchao @sadikovi
I've completed most part of arrow reader and code is here: https://github.com/liurenjie1024/arrow/tree/arrow-4059/rust/parquet/src/arrow
Hope it will help reviewers to understand what record_reader.rs is doing.
I don't submit the whole reader in a huge PR because I think small PRs is more friendly for reviewing, so please take a look at this.

Nice @liurenjie1024 ! will take a look at this.

Codecov Report

❗ No coverage uploaded for pull request base (master@f35dd92). Click here to learn what that means.
The diff coverage is 89.76%.

@@            Coverage Diff            @@
##             master    #4292   +/-   ##
=========================================
  Coverage          ?   82.73%           
=========================================
  Files             ?       87           
  Lines             ?    25430           
  Branches          ?        0           
=========================================
  Hits              ?    21040           
  Misses            ?     4390           
  Partials          ?        0

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f35dd92...874bb23. Read the comment docs.

@liurenjie1024 thanks again for the POC (sorry for being late as I was on vacation). I think I have a much better understanding on this now. Could you address the comments I had and update this PR?

@sunchao Sure, will fix it soon

@sunchao Fixed comments. Please take a look when you got a chance.

@sunchao Thanks for the review. Fixed comments. Will take a look at the ci error later.

Thanks @liurenjie1024 . The PR looks good to me except one minor thing: can you rename records_num to num_records in the RecordReader struct? After the CI issue is resolved I think the PR can be merged.

@sunchao CI passed. I think the c++ build failure is unrelated.