Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ArrowReaderMetadata API makes it too easy to (accidentally) make an additional object store request #6476

Open
alamb opened this issue Sep 28, 2024 · 3 comments
Labels
enhancement Any new improvement worthy of a entry in the changelog

Comments

@alamb
Copy link
Contributor

alamb commented Sep 28, 2024

Is your feature request related to a problem or challenge? Please describe what you are trying to do.

ArrowReaderMetadata to read parquet files, and one major usecase is to supply pre-parsed metadata (to avoid a second object store request on read) by providing the ParquetMetaData to ArrowReaderMetadata::try_new

However, the way the API is currently setup it is easy to supply the ParquetMetaData but the reader will STILL make 2 object store requests.

This happens if the ArrowReaderOptions has with_page_index specified but the provided metadata doesn't (yet) have the page index, it will load it again

This is a common source of confusion / bugs: when someone supplies the ParquetMetaData to the ArrowReaderMetadata they are very often trying to avoid a second object store request, but as it often turns out the second fetch happens anyways to read the page index (thus obviating the attempt at optimization)

This is (in a roundabout way) what is happening to @progval in apache/datafusion#12593 and it took me a while to debug what was happening while working on the advanced_parquet_index.rs in DataFusion

Describe the solution you'd like
I would like the API to be harder to misuse.

Describe alternatives you've considered
For example, maybe we could make ArrowReaderMetadata error if it was supplied with ParquetMetaData that did not have the page indexes,

for example, we could add a ArrowReaderOptions::error_if_need_metadata or something that would change the automatic fetch/load behavior into an error if the reader needs the page index, and the file has a page index, but it isn't loaded yet into ParquetMetaData

Additional context

@alamb alamb added the enhancement Any new improvement worthy of a entry in the changelog label Sep 28, 2024
@doki23
Copy link
Contributor

doki23 commented Oct 7, 2024

I propose another approach: we could cache the page index upon its initial load ( make user provided metadata mutable), as the current API seems satisfactory to me.

@alamb
Copy link
Contributor Author

alamb commented Oct 8, 2024

I propose another approach: we could cache the page index upon its initial load ( make user provided metadata mutable), as the current API seems satisfactory to me.

I am not quite sure what you mean by this -- depending on how you setup the initial load, the page index will be read. The issue I was trying to describe is if you don't read the page index in the initial load, a subsequent opening of the file even if you supply ParquetMetaData, will force another fetch of metadata (page index) prior to reading the data

@doki23
Copy link
Contributor

doki23 commented Oct 9, 2024

I propose another approach: we could cache the page index upon its initial load ( make user provided metadata mutable), as the current API seems satisfactory to me.

I am not quite sure what you mean by this -- depending on how you setup the initial load, the page index will be read. The issue I was trying to describe is if you don't read the page index in the initial load, a subsequent opening of the file even if you supply ParquetMetaData, will force another fetch of metadata (page index) prior to reading the data

Thank you, get it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Any new improvement worthy of a entry in the changelog
Projects
None yet
Development

No branches or pull requests

2 participants