vaibhavbodhane commented on code in PR #26920:
URL: https://github.com/apache/datafusion/pull/26920#discussion_r4440063877
##########
datafusion/datasource-parquet/src/reader.rs:
##########
@@ -77,6 +77,32 @@ pub trait ParquetFileReaderFactory: Debug + Send + Sync +
'static {
metadata_size_hint: Option<usize>,
metrics: &ExecutionPlanMetricsSet,
) -> datafusion_common::Result<Box<dyn AsyncFileReader + Send>>;
+
+ /// Like [`Self::create_reader`], but the reader should record into
+ /// `file_metrics`, the [`ParquetFileMetrics`] the caller has already
+ /// registered for this file, rather than registering another set in
+ /// `metrics`.
+ ///
+ /// The parquet opener calls this so that each file it opens registers one
+ /// set of file metrics. The default implementation ignores `file_metrics`
+ /// and calls [`Self::create_reader`], so existing implementations keep
+ /// their behavior.
+ fn create_reader_with_file_metrics(
Review Comment:
Thanks @alamb, agreed. An argument struct is a better API than another
positional parameter. I'll rework the PR along these lines:
- `CreateReaderArgs` with private fields and a builder:
`CreateReaderArgs::new(partition_index, partitioned_file, metrics)`, plus
`with_metadata_size_hint(..)` and `with_file_metrics(ParquetFileMetrics)`, and
getters.
- A new trait method, `create_reader_with_args(&self, args:
CreateReaderArgs)`. Its default implementation calls `create_reader`, so
existing third-party factories keep working unchanged.
- `create_reader` deprecated. Its default implementation returns a
not-implemented error, so new factories only need to implement
`create_reader_with_args`. Both methods can't default to each other, because a
factory that implemented neither would recurse forever.
- The built-in factories and both opener call sites use
`create_reader_with_args`, passing the file's existing `ParquetFileMetrics`, so
the fix itself is unchanged.
I'll also rebase on main. Let me know if you'd rather keep `create_reader`
undeprecated for now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
