ARROW-7681: [Rust] Explicitly seeking a BufReader will discard the internal buffer (2)

[rdettai]

A fix to 7681 that does not use nightly (as oposed to #6280).

[github-actions]

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

[rdettai]

I would like to add a test on a parquet file with many lines because my first commit passed all tests but was buggy... Where should I add this file ?

[paddyhoran]

Where should I add this file ?

The short answer is here but you should mention it on the mailing list first. There is reluctance to adding binary data I believe. The preference is to make progress on the writer implementation so that any test files can be generated. There has been some initial progress on this here.

If there is reluctance to add the file you can comment out the test and open up a JIRA to follow up once the writer is implemented I guess.

[paddyhoran]

Changes look reasonable to me BTW, but they need to be reviewed by @sunchao or @sadikovi in general as they are the experts on Parquet.

[rdettai]

In the end I did not go for the end to end test with a Parquet file as this is kind of a more general concern. I added a "byte level" test though to maintain good coverage!

Hope the MacOS CI will not go nuts this time ;-)

[sunchao]

Sorry just noticed this! LGTM with a few nits

[sunchao]

This is not super useful as it requires ppl to jump into BufReader to check the documentation - maybe add additional comments on what these two fields are for?

[sunchao]

nit: maybe we can extract this into a util function?

[rdettai]

I'll work on your comments today.

What about the problem we are trying to fix here? Do you agree with the benefits of this fix ?

Also, I'm not sure why a Mutex was used here. Do we agree that Refcell is better as the whole reader is not thread safe, right ?

[sunchao]

Yes I think it is beneficial to avoid dropping buffers with seek, although it will be nice if the seek_relative will be stabilized soon so we can just use that.

Also, I'm not sure why a Mutex was used here. Do we agree that Refcell is better as the whole reader is not thread safe, right ?

Originally we designed it this way so that we can concurrently read multiple column chunks after obtaining file handle from a single row group. Since the file handle is shared between these we wanted to provide thread safety on top of it.

[rdettai]

Originally we designed it this way so that we can concurrently read multiple column chunks after obtaining file handle from a single row group. Since the file handle is shared between these we wanted to provide thread safety on top of it.

I'm not sure to understand how this could be the concern of the FileSource. It's the reader (file handle) that is passed to it that should be thread safe, and FileSource should not create the Mutex himself.

[sunchao]

It's the reader (file handle) that is passed to it that should be thread safe

Is file thread-safe? it's not obvious when reading the doc. Plus, here type parameterR can be anything that implements ParquetReader (with read, seek, length and try_clone capabilities) so you cannot assume all of them guarantees thread-safety.

[rdettai]

I completely agree ! What I am saying is that the layer that makes the handle thread safe cannot be the FileSource which is in charge of tracking the reading position of one specific reading thread.

But this question of multi-threading seems to be a whole other concern :-) I've made the nit changes you've proposed and improved the tests.

[sunchao]

Merged. Thanks @rdettai !

[jorisvandenbossche]

It seems that linting for rust was failing here? (and now on master)

[nevi-me]

It seems that linting for rust was failing here? (and now on master)

I'll fix it shortly @jorisvandenbossche

[sunchao]

Oops. Sorry my bad. Thanks @nevi-me !

[rdettai]

My bad, I thought it was an issue with the linting CI !