Deprecate `MetadataLoader` #6474

etseidl · 2024-09-27T17:27:53Z

Which issue does this PR close?

Part of #6447

Rationale for this change

What changes are included in this PR?

Deprecate public methods in MetadataLoader, as well as metadata::fetch_parquet_metadata().

Also changes the load API in ParquetMetaDataReader to use references to the MetadataFetch rather than taking ownership.

Are there any user-facing changes?

Yes, but only to the as yet unreleased ParquetMetaDataReader.

etseidl · 2024-09-27T17:31:11Z

cc @alamb. I did not like that the load... functions in ParquetMetaDataReader took ownership of fetch. This made sense in MetadataLoader because the fetch object was used multiple times within the loader object. It took me some time to figure out the correct incantation to allow passing references 😅. Please take a close look at the changes in reader.rs to see if you agree with how I've done it. 🙏

alamb

Thanks @etseidl -- I love the deprecation, but am not sure about the change to use &mut F references. I think the owned API makes more sense

alamb · 2024-09-28T09:21:00Z

parquet/src/arrow/async_reader/metadata.rs

 }

+// these tests are all replicated in parquet::file::metadata::reader


alamb · 2024-09-28T09:25:06Z

parquet/src/arrow/async_reader/mod.rs

@@ -212,16 +212,18 @@ impl ArrowReaderMetadata {
        input: &mut T,
        options: ArrowReaderOptions,
    ) -> Result<Self> {
+        // TODO: this is all rather awkward. It would be nice if AsyncFileReader::get_metadata


Not only is this akward, it is also a common source of confusion / bugs (namely that when someone supplies the ParquetMetaData to the arrow reader options to avoid a second object store request, if often turns out the second fetch happens anyways to read the page index (thus obviating the attempt at optimization)

To avoid this they need to ensure when they read the metadata in the first place, they also read the page index.

This is (in a roundabout way) what is happening to @progval in apache/datafusion#12593

I will try and file a ticket explaining the issue

Filed #6476

alamb · 2024-09-28T09:25:56Z

parquet/src/file/metadata/reader.rs

@@ -299,11 +299,14 @@ impl ParquetMetaDataReader {
    /// See [`Self::with_prefetch_hint`] for a discussion of how to reduce the number of fetches
    /// performed by this function.
    #[cfg(feature = "async")]
-    pub async fn load_and_finish<F: MetadataFetch>(
+    pub async fn load_and_finish<F>(


since this API is new and has not yet been released, this is not a breaking API change

(Specifically ParquetMetaDataReader has not been released yet)

Which is why I was rushing to get this into 53.1.0. 😄 But if we revert this change then this PR can merge after 53.1.0 is released.

alamb · 2024-09-28T09:28:49Z

parquet/src/arrow/async_reader/store.rs

-            let mut loader = MetadataLoader::load(self, file_size, prefetch).await?;
-            loader
-                .load_page_index(preload_column_index, preload_offset_index)
+            let metadata = ParquetMetaDataReader::new()


alamb · 2024-09-28T09:52:21Z

parquet/src/file/metadata/reader.rs

+    where
+        for<'a> &'a mut F: MetadataFetch,


cc @alamb. I did not like that the load... functions in ParquetMetaDataReader took ownership of fetch.

What don't you like about it? Is the idea that MetadataFetch actually has mutable state so that calling fetch would change its state?

I think in general trying to get async functions to be happy with references is often much harder than with owned objects.

So I guess I think we should keep the API as owned to make working with these APIs simpler, unless there is some compelling reason for the change (better performance, etc)

What don't you like about it? Is the idea that MetadataFetch actually has mutable state so that calling fetch would change its state?

Some of it is my lack of Rust experience. I'm having a hard time tracking the ownership here, and am confusing traits with objects. Implementing the MetadataFetch trait for a mutable reference to something that implements the AsyncFileReader trait leaves my head spinning a little.

I think in general trying to get async functions to be happy with references is often much harder than with owned objects.

Indeed, it took me days to get this to compile 😅.

So I guess I think we should keep the API as owned to make working with these APIs simpler, unless there is some compelling reason for the change (better performance, etc)

I'll revert now that I understand the mechanics a little better. But one consequence is some things will get a little more verbose. For instance, when not taking ownership, get_metadata for ParquetObjectReader is

fn get_metadata(&mut self) -> BoxFuture<'_, Result<Arc<ParquetMetaData>>> { Box::pin(async move { let metadata = ParquetMetaDataReader::new() .with_column_indexes(self.preload_column_index) .with_offset_indexes(self.preload_offset_index) .with_prefetch_hint(self.metadata_size_hint) .load_and_finish(self, self.meta.size) .await?; Ok(Arc::new(metadata)) }) }

But if load_and_finish takes ownership of the mutable reference to self, we can't then get at self.meta.size in the same call, but need to save it earlier, IIUC.

fn get_metadata(&mut self) -> BoxFuture<'_, Result<Arc<ParquetMetaData>>> { Box::pin(async move { let file_size = self.meta.size; let metadata = ParquetMetaDataReader::new() .with_column_indexes(self.preload_column_index) .with_offset_indexes(self.preload_offset_index) .with_prefetch_hint(self.metadata_size_hint) .load_and_finish(self, file_size) .await?; Ok(Arc::new(metadata)) }) }

A small price to pay, but one I was hoping to avoid.

A small price to pay, but one I was hoping to avoid.

I agree it would be nicer.

While I don't fully understand all the implications here, this is in my mind related to how how the rust compiler generates async continuations and how it interacts with the borrow checker. Sometimes it has a hard time expressing that a borrow is ok for some reason

etseidl · 2024-09-28T19:22:19Z

parquet/src/file/metadata/reader.rs

    }

    /// Asynchronously fetch the page index structures when a [`ParquetMetaData`] has already
    /// been obtained. See [`Self::new_with_metadata()`].
    #[cfg(feature = "async")]
-    pub async fn load_page_index<F: MetadataFetch>(
+    pub async fn load_page_index<F: MetadataFetch>(&mut self, fetch: F) -> Result<()> {


This change will be breaking if in 52.2.0. I'll revert this if need be. I found having remainder in the public API confusing.

etseidl · 2024-09-28T20:19:17Z

Thanks for the quick review @alamb

alamb

Thank you @etseidl -- this looks great to me. I think the tests can be simplified, but I will make a follow on PR with a proposal.

alamb · 2024-09-30T17:58:19Z

parquet/src/file/metadata/reader.rs

@@ -836,7 +841,7 @@ mod async_tests {

    struct MetadataFetchFn<F>(F);

-    impl<F, Fut> MetadataFetch for MetadataFetchFn<F>
+    impl<'a, F, Fut> MetadataFetch for &'a mut MetadataFetchFn<F>


I think now that the API has been changed back, these test changes are also not needed

This change allows me to wrap the fetch function once, rather than for each invocation of load, but that could be done inline or with a From I suppose. Happy to revert if you'd like.

I thought reverting them was nice to demonstrate the same API can still be used

I did notice that the metadata loader tests actually have a copy/paste MetadataFetchFn adapter. Maybe if the arrow crate needs it in two places, we should make it easier for actual users to write them 🤔 Not sure

etseidl added 4 commits September 26, 2024 15:12

deprecate MetadataLoader

5843f45

change signature of the load functions

cb1498a

fix up fetch_parquet_metadata

5f07723

can now use self.meta.size directly

7e669cc

github-actions bot added the parquet Changes to the parquet crate label Sep 27, 2024

etseidl mentioned this pull request Sep 27, 2024

Deprecate and replace old Parquet metadata parsing functions #6447

Open

6 tasks

alamb reviewed Sep 28, 2024

View reviewed changes

etseidl added 2 commits September 28, 2024 12:04

revert changes to load API

8068a31

revert change to test code

388cf5a

etseidl commented Sep 28, 2024

View reviewed changes

alamb approved these changes Sep 30, 2024

View reviewed changes

alamb merged commit 3293a8c into apache:master Sep 30, 2024
16 checks passed

etseidl deleted the deprecate_metadata_loader branch September 30, 2024 18:11

alamb mentioned this pull request Sep 30, 2024

Cleanup MetadataLoader tests #6484

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate `MetadataLoader` #6474

Deprecate `MetadataLoader` #6474

etseidl commented Sep 27, 2024

etseidl commented Sep 27, 2024

alamb left a comment

alamb Sep 28, 2024

alamb Sep 28, 2024

alamb Sep 28, 2024

alamb Sep 28, 2024

alamb Sep 28, 2024

etseidl Sep 28, 2024

alamb Sep 28, 2024

alamb Sep 28, 2024

etseidl Sep 28, 2024 •

edited

Loading

alamb Sep 30, 2024

etseidl Sep 28, 2024

etseidl commented Sep 28, 2024

alamb left a comment

alamb Sep 30, 2024

etseidl Sep 30, 2024

alamb Sep 30, 2024 •

edited

Loading

alamb Sep 30, 2024

		}

		// these tests are all replicated in parquet::file::metadata::reader

Deprecate MetadataLoader #6474

Deprecate MetadataLoader #6474

Conversation

etseidl commented Sep 27, 2024

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

etseidl commented Sep 27, 2024

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etseidl Sep 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etseidl commented Sep 28, 2024

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Deprecate `MetadataLoader` #6474

Deprecate `MetadataLoader` #6474

etseidl Sep 28, 2024 •

edited

Loading

alamb Sep 30, 2024 •

edited

Loading