Use BufReader for LocalFileReader to revert performance regression in parquet reading

[Dandandan]

Which issue does this PR close?

Closes #1363

Rationale for this change

Parquet reading was slow. This recovers the performance regression in the TPC-H benchmark.

There is still a slowdown in query 10 - and other queries, but this is unrelated to Parquet reading #1367 (and performance still improves from roughly 10 to 7s on that query).

What changes are included in this PR?

Use the BufReadtrait instead ofRead`.

Are there any user-facing changes?

[alamb]

Thank you @Dandandan -- I am not sure about the need for the BufRead trait, but otherwise looks good to me.

Very nice 🕵️ work

[alamb]

TIL BufRead trait

[alamb]

It seems like this is the actual fix, right? Is the change to require the BufRead trait needed?

[Dandandan]

Not sure. Rerunning some benchmarks now without the trait.

[Dandandan]

Yes - looks like BufRead wasn't needed 🎉

[xudong963]

🎉

[rdettai]

Thanks @Dandandan ! Can you quickly explain what the reason for the slowdown was exactly?

[Dandandan]

Thanks @Dandandan ! Can you quickly explain what the reason for the slowdown was exactly?

As far as I can explain:

The earlier code used the parquet-based API to read from a file, which uses a BufReader internally, which is crucial for the performance.

By introducing the object storage abstraction, we were directly reading from a File instance without any buffering in between, i.e. having lot's of extra calls to the OS (as you also hinted at in #1363).
This leads to both slowdown in loading the data but also was very expensive in the part that reads metadata /statistics (which normally takes something like <1ms locally). Probably that part does many small read calls.

By wrapping the File instance in the BufReader we avoid those calls to the OS.

Maybe a potential improvement would be having a bit more control, such as setting the capacity of the buffer.