I plan to review this later today

I looked a bit at https://gitlab.softwareheritage.org/swh/devel/swh-provenance/-/merge_requests/182/diffs

I wonder if you could cache the Arc rather than the ArrowReaderMetadta 🤔 (I find the maze of similarly named "Reader/loader/Arrow/metadata" structures hard to keep straight sometimes

I agree, it's confusing.

I actually started from advanced_parquet_index.rs, but it appeared to be insufficient.

But reading this again, I agree with you that it should be. I'll give it another shot.

I remembered / rediscovered the issue. It's in this code:

https://github.com/apache/arrow-rs/blob/62825b27e98e6719cb66258535c75c7490ddba44/parquet/src/arrow/async_reader/mod.rs#L212-L228

ArrowReaderMetadata::load_metadata does two expensive things:

1. call AsyncFileReader::get_metadata() to get a ParquetMetadata, which we can cache as per advanced_parquet_index.rs
2. instantiate and use parquet::MetadataLoader to get another ParquetMetadata from the first one. And this new ParquetMetadata isn't cachable without my patch.

in essence, it's enriching the metadata struct by doing: let metadata = MetadataLoader::new(..., metadata).load_page_index(...).finish()

To give an idea of the difference. Metrics with the new load_metadata cached:

time_elapsed_opening=165.253279952s, time_elapsed_processing=36.211909412s, time_elapsed_scanning_total=605.765021ms, time_elapsed_scanning_until_data=605.726082ms

Metrics with only get_metadata cached:

time_elapsed_opening=272.811692949s, time_elapsed_processing=118.675115374s, time_elapsed_scanning_total=2.530368545s, time_elapsed_scanning_until_data=2.530322168s

I wonder if we could change the "automatically load page index if needed" to "error if page index is needed but it is not loaded" 🤔 That might be a less surprising behavior

I wonder if we could change the "automatically load page index if needed" to "error if page index is needed but it is not loaded" 🤔 That might be a less surprising behavior

I agree, and it would work for my use case because I always want the page index. I did it like this:

     fn get_metadata(&mut self) -> BoxFuture<'_, parquet::errors::Result<Arc<ParquetMetaData>>> {
-        self.inner.get_metadata()
+        Box::pin(self.get_metadata_async())
     }
 
     fn get_byte_ranges(
@@ -117,6 +118,55 @@ impl AsyncFileReader for CachingParquetFileReader {
     }
 }
 
+impl CachingParquetFileReader {
+    /// Implementation of [`AsyncFileReader::get_metadata`] using new-style async,
+    /// so it can pass the borrow checker
+    async fn get_metadata_async(&mut self) -> parquet::errors::Result<Arc<ParquetMetaData>> {
+        match &self.metadata {
+            Some(metadata) => Ok(Arc::clone(metadata)),
+            None => match self.inner.get_metadata().await {
+                Ok(metadata) => {
+                    // This function is called by `ArrowReaderMetadata::load_async`.
+                    // Then, `load_async` may enrich the `ParquetMetaData` we return with
+                    // the page index, using `MetadataLoader`; and this enriched
+                    // `ParquetMetaData` reader would not be cached.
+                    //
+                    // Datafusion does not (currently) support caching the enriched
+                    // `ParquetMetaData`, so we unconditionally enrich it here with
+                    // the page index, so we can cache it.
+                    //
+                    // See:
+                    // * discussion on https://github.com/apache/datafusion/pull/12593
+                    // * https://github.com/apache/arrow-rs/blob/62825b27e98e6719cb66258535c75c7490ddba44/parquet/src/arrow/async_reader/mod.rs#L212-L228
+                    let metadata = Arc::try_unwrap(metadata).unwrap_or_else(|e| e.as_ref().clone());
+                    let mut loader = MetadataLoader::new(
+                        CachingParquetFileReaderMetadataFetch(self),
+                        metadata,
+                    );
+                    loader.load_page_index(true, true).await?;
+                    let metadata = Arc::new(loader.finish());
+                    self.metadata = Some(Arc::clone(&metadata));
+                    Ok(metadata)
+                }
+                Err(e) => Err(e),
+            },
+        }
+    }
+}
+
+struct CachingParquetFileReaderMetadataFetch<'a>(&'a mut CachingParquetFileReader);
+
+impl<'a> MetadataFetch for CachingParquetFileReaderMetadataFetch<'a> {
+    fn fetch(
+        &mut self,
+        range: Range<usize>,
+    ) -> BoxFuture<'_, parquet::errors::Result<bytes::Bytes>> {
+        println!("fetch");
+        self.0.fetch(range)
+    }
+}

(For some reason it's a bit slower even though it does cache as it should. I'll investigate in two weeks)

But for a generic solution, the FileOpener would need to tell AsyncFileReader::get_metadata (through ArrowReaderMetadata) whether the page index is needed, so this needs an API change (or at least addition) to Arrow. But since the parquet crate is being refactored, this may be the right time.

Thanks @progval -- I will find time to review this more carefully tomorrow.

My primary concerns are with the API being even more complicated to use correctly than the current one (the upcast thing is confusing and "feels" wrong to me, as does the need for another API in DataFusion that wraps an already complicated API in parquet.

I am hopeful we can come up with something more elegant

(For some reason it's a bit slower even though it does cache as it should. I'll investigate in two weeks)

Actually, it's not slower. I must have had a noisy benchmark or bad luck eyeballing the mean request time.