Fix file buffer reuse when getting page readers

[sadikovi]

This PR fixes an interesting issue with file handle reuse, and results in not being able to buffer values from column readers, unless reading entire column in a row group at a time.

Problem description

Discovered this problem when building a batch of column vectors. The basic idea is that when initialising column readers (page readers), we clone file handles for each page reader (line 212 in src/file/reader.rs), so they can be traversed independently from each other.

Unfortunately, try_clone has an interesting side-effect: The returned File is a reference to the same state that this object references. Both handles will read and write with the same cursor position (https://doc.rust-lang.org/std/fs/struct.File.html#method.try_clone). So when updating one, we update the other. More info here: issue 46578 (https://github.com/rust-lang/rust/issues).

This bug is observed when reading more than one column. I added a simple test to reproduce the problem (fails on master, passes with this patch). The position will be overwritten and all column readers will be using handle of the last one essentially. This results in different weird bugs, one of them is underlying Thrift error: end of file.

Fix

It looks like standard library does not have any handles that maintain positions separately, unless you want to open file again, which I assume we do not want to. I also considered buffering each column into memory (essentially the whole row group) - copying bytes from a file (col_start, col_length) into some vector of bytes. This also does not seem like a way to go.

So I added a simple struct that takes file handle from file.try_clone() and maintains start and end of the chunk of the file and updates them as we read it. Every time when we read FileChunk, we update file position on the current file handle position - also added Mutex for concurrent access.

This seems to work and passes unit tests. I also tested manually with different files - all works correctly.

[sadikovi]

@sunchao Could you review this PR? I am interested in your thoughts on the issue and if you know any other way of fixing this - my approach requires updating start position for each read, I assume it is not particularly performant.

[sadikovi]

Looks like issue with compiling thrift. Tests pass locally.

[sadikovi]

@sunchao Meanwhile, when you have time, could you have look at this pull request? Thanks.

[sunchao]

Yes. Will take a look at this soon. Sorry for the delay.

[sadikovi]

No worries, it is all good. Whenever you have time:)

[sunchao]

Thanks @sadikovi ! This is an very interesting finding.

The new FileHandle looks fine. However, I'm wondering whether it will be easier to just create multiple handles via File::open(..). Instead of storing and passing BufReader, we can pass PathBuf to row group and column readers. What is your opinion?

[sadikovi]

Hi! I thought about it, but was not sure if it is okay to have several File::open calls, seek vs having separate handles. I am curious how parquet-cop version works, and whether or not they have the same issue. We could employ the same technique.

I can fix it either way. I tend to agree that FileHandle might be a better approach with minimum amount of changes. Otherwise, we would have to open file for every column we read.

Again, I will look on how parquet-cpp handles it, and report back.

[sadikovi]

Parquet-cpp use input stream with start and length tracking. My understanding is that what they do is similar to what this patch is proposing.

@sunchao Could you help me to figure out the code below and whether it is the same as what we are doing in this code?:) Thanks!

See (file_reader -> properties -> input stream):
https://github.com/apache/parquet-cpp/blob/c405bf36506ec584e8009a6d53349277e600467d/src/parquet/file_reader.cc#L124

https://github.com/apache/parquet-cpp/blob/3b34f02d53db60419587850d04e7ea2abf1794dd/src/parquet/properties.h#L51

https://github.com/apache/parquet-cpp/blob/4b09ac703bc75fee72f94bed8ecfe571096b04c1/src/parquet/util/memory.cc#L455

The only concern I have is whether or not this approach of resetting start position has any performance implications. If not - then it is okay.

[sunchao]

Thanks @sadikovi . Yes, I think our approach is very similar to what Parquet-cpp/Arrow does. One thing we should be careful is thread safety between multiple shared Files - Arrow uses a lock to protect concurrent access: https://github.com/apache/arrow/blob/master/cpp/src/arrow/io/interfaces.cc#L42.

[sadikovi]

Yes, you are right. I will try updating the code similar to parquet-cpp/arrow for concurrent access.

[sadikovi]

Will get back to it ASAP - needed for read API!

[sadikovi]

@sunchao Could you review this PR again? I renamed the struct into FileChunk and added Mutex on buf reader. I hope this aligns with your comment on concurrent reads - if not, let me know, I will update the code. I am not sure if there is a performance hit when using mutex for a read... 🤔.

I also added couple of tests, but I had to copy get_test_file() method. Hope it is not a big deal:)

[coveralls]

Coverage increased (+0.2%) to 92.484% when pulling 5466529 on sadikovi:fix-file-clone into 4c07f35 on sunchao:master.

[sunchao]

Thanks @sadikovi . This looks good - just a few minor comments.

[sunchao]

nit: I think this error is still possible to occur - should we convert to a IO Result?

[sadikovi]

Yes, you are right. We should turn it into Result.

[sunchao]

Should we rename this to test_reuse_file_chunk?

[sunchao]

nit: initialising -> initializing

[sadikovi]

@sunchao I addressed your comments. Can you have a look again when you have time? Thanks.

[sunchao]

LGTM!

[sunchao]

I've merged the change. Thanks @sadikovi !

[sadikovi]

@sunchao Thanks a lot for merging!