You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "alamb (via GitHub)" <gi...@apache.org> on 2024/06/09 21:28:05 UTC
[PR] WIP Prototype DataPage extraction API [datafusion]
alamb opened a new pull request, #10843:
URL: https://github.com/apache/datafusion/pull/10843
## Which issue does this PR close?
Part of https://github.com/apache/datafusion/issues/10806
## Rationale for this change
@marvinlanhenke asked some excellent questions on https://github.com/apache/datafusion/issues/10806#issuecomment-2156778662
I needed to try out a few things myself so I figured rather than
## What changes are included in this PR?
1. Sketch out a `StatisticsExtractor` API, and a somewhat jenky implementation
2. Update the `arrow_statistics` tests to have an option to test data page statistics
## Are these changes tested?
Yes
## Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] WIP Prototype DataPage extraction API [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #10843:
URL: https://github.com/apache/datafusion/pull/10843#issuecomment-2159591102
I think this is now inorporated into https://github.com/apache/datafusion/pull/10852
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] WIP Prototype DataPage extraction API [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #10843:
URL: https://github.com/apache/datafusion/pull/10843#discussion_r1632395830
##########
datafusion/core/src/datasource/physical_plan/parquet/statistics.rs:
##########
@@ -766,10 +769,108 @@ impl<'a> StatisticsConverter<'a> {
Ok(Arc::new(UInt64Array::from_iter(null_counts)))
}
- /// Returns a null array of data_type with one element per row group
- fn make_null_array<I>(&self, data_type: &DataType, metadatas: I) -> ArrayRef
+ /// Extract the minimum values from Data page statistics
+ ///
+ /// In Parquet files, in addition to the Column Chunk level statistics
+ /// (stored for each column for each row group) there are also optional
+ /// statistics stored for each data page, part of the [Parquet Page Index].
+ /// Since a single Column Chunk is stored as one or more pages, page level statistics
+ /// can prune at a finer granularity.
+ ///
+ /// However since they are stored in a separate metadata structure
+ /// ([`Index`]) there is different code to extract them as arrow statistics
+ ///
+ /// Parameters:
+ ///
+ /// * `page_index`: The parquet page index, likely read from
+ /// [`ParquetMetadata::page_index()`]
+ ///
+ /// * row_group_indexes: The indexes of the row groups (indexes in
+ /// `page_index`) to extract the statistics from. This is an interator over `&usize` to
+ /// permit passing in `&Vec<usize>` or similar
+ ///
+ /// # Return Value
+ ///
+ /// The returned array contains 1 value for each `NativeIndex` in the underlying
+ /// `Index`es, in the same order as they appear in `metadatas`.
+ ///
+ /// For example, if there are two `Index`es in `metadatas`:
+ /// 1. the first having `3` `PageIndex` entries
+ /// 2. the second having `2` `PageIndex` entries
+ ///
+ /// The returned array would have 5 rows
+ ///
+ /// Each value is either
+ /// * the minimum value for the page
+ /// * a null value, if the statistics can not be extracted
+ ///
+ /// Note that a null value does NOT mean the min value was actually
+ /// `null` it means it the requested statistic is unknown
+ ///
+ /// # Errors
+ ///
+ /// Reasons for not being able to extract the statistics include:
+ /// * the column is not present in the parquet file
+ /// * statistics for the pages are not present in the row group
+ /// * the stored statistic value can not be converted to the requested type
+ ///
+ /// # Example
+ /// ```no_run
+ /// tood
+ /// ```
+ pub fn data_page_mins<I>(
Review Comment:
This is one way the API could look like
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] WIP Prototype DataPage extraction API [datafusion]
Posted by "marvinlanhenke (via GitHub)" <gi...@apache.org>.
marvinlanhenke commented on code in PR #10843:
URL: https://github.com/apache/datafusion/pull/10843#discussion_r1633318039
##########
datafusion/core/src/datasource/physical_plan/parquet/statistics.rs:
##########
@@ -766,10 +769,108 @@ impl<'a> StatisticsConverter<'a> {
Ok(Arc::new(UInt64Array::from_iter(null_counts)))
}
- /// Returns a null array of data_type with one element per row group
- fn make_null_array<I>(&self, data_type: &DataType, metadatas: I) -> ArrayRef
+ /// Extract the minimum values from Data page statistics
+ ///
+ /// In Parquet files, in addition to the Column Chunk level statistics
+ /// (stored for each column for each row group) there are also optional
+ /// statistics stored for each data page, part of the [Parquet Page Index].
+ /// Since a single Column Chunk is stored as one or more pages, page level statistics
+ /// can prune at a finer granularity.
+ ///
+ /// However since they are stored in a separate metadata structure
+ /// ([`Index`]) there is different code to extract them as arrow statistics
+ ///
+ /// Parameters:
+ ///
+ /// * `page_index`: The parquet page index, likely read from
+ /// [`ParquetMetadata::page_index()`]
+ ///
+ /// * row_group_indexes: The indexes of the row groups (indexes in
+ /// `page_index`) to extract the statistics from. This is an interator over `&usize` to
+ /// permit passing in `&Vec<usize>` or similar
+ ///
+ /// # Return Value
+ ///
+ /// The returned array contains 1 value for each `NativeIndex` in the underlying
+ /// `Index`es, in the same order as they appear in `metadatas`.
+ ///
+ /// For example, if there are two `Index`es in `metadatas`:
+ /// 1. the first having `3` `PageIndex` entries
+ /// 2. the second having `2` `PageIndex` entries
+ ///
+ /// The returned array would have 5 rows
+ ///
+ /// Each value is either
+ /// * the minimum value for the page
+ /// * a null value, if the statistics can not be extracted
+ ///
+ /// Note that a null value does NOT mean the min value was actually
+ /// `null` it means it the requested statistic is unknown
+ ///
+ /// # Errors
+ ///
+ /// Reasons for not being able to extract the statistics include:
+ /// * the column is not present in the parquet file
+ /// * statistics for the pages are not present in the row group
+ /// * the stored statistic value can not be converted to the requested type
+ ///
+ /// # Example
+ /// ```no_run
+ /// tood
+ /// ```
+ pub fn data_page_mins<I>(
+ &self,
+ page_index: &ParquetColumnIndex,
+ row_group_indexes: I,
Review Comment:
I had this interface in some version of my prototype, where it would take an iterator over all row_groups directly. However, this cannot be easily integrated with the existing code and the access_plan.row_group_indices. Perhaps, once the StatisticsConverter is fully used in `page_filter.rs` we can change the interface?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] WIP Prototype DataPage extraction API [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #10843:
URL: https://github.com/apache/datafusion/pull/10843#discussion_r1632396117
##########
datafusion/core/tests/parquet/arrow_statistics.rs:
##########
@@ -1724,7 +1808,7 @@ async fn test_boolean() {
.build()
.await;
- Test {
+ TestBoth {
Review Comment:
By changing a test to `TestBoth` it will test both row group indexes and data page indexes
##########
datafusion/core/src/datasource/physical_plan/parquet/statistics.rs:
##########
@@ -766,10 +769,108 @@ impl<'a> StatisticsConverter<'a> {
Ok(Arc::new(UInt64Array::from_iter(null_counts)))
}
- /// Returns a null array of data_type with one element per row group
- fn make_null_array<I>(&self, data_type: &DataType, metadatas: I) -> ArrayRef
+ /// Extract the minimum values from Data page statistics
+ ///
+ /// In Parquet files, in addition to the Column Chunk level statistics
+ /// (stored for each column for each row group) there are also optional
+ /// statistics stored for each data page, part of the [Parquet Page Index].
+ /// Since a single Column Chunk is stored as one or more pages, page level statistics
+ /// can prune at a finer granularity.
+ ///
+ /// However since they are stored in a separate metadata structure
+ /// ([`Index`]) there is different code to extract them as arrow statistics
+ ///
+ /// Parameters:
+ ///
+ /// * `page_index`: The parquet page index, likely read from
+ /// [`ParquetMetadata::page_index()`]
+ ///
+ /// * row_group_indexes: The indexes of the row groups (indexes in
+ /// `page_index`) to extract the statistics from. This is an interator over `&usize` to
+ /// permit passing in `&Vec<usize>` or similar
+ ///
+ /// # Return Value
+ ///
+ /// The returned array contains 1 value for each `NativeIndex` in the underlying
+ /// `Index`es, in the same order as they appear in `metadatas`.
+ ///
+ /// For example, if there are two `Index`es in `metadatas`:
+ /// 1. the first having `3` `PageIndex` entries
+ /// 2. the second having `2` `PageIndex` entries
+ ///
+ /// The returned array would have 5 rows
+ ///
+ /// Each value is either
+ /// * the minimum value for the page
+ /// * a null value, if the statistics can not be extracted
+ ///
+ /// Note that a null value does NOT mean the min value was actually
+ /// `null` it means it the requested statistic is unknown
+ ///
+ /// # Errors
+ ///
+ /// Reasons for not being able to extract the statistics include:
+ /// * the column is not present in the parquet file
+ /// * statistics for the pages are not present in the row group
+ /// * the stored statistic value can not be converted to the requested type
+ ///
+ /// # Example
+ /// ```no_run
+ /// tood
+ /// ```
+ pub fn data_page_mins<I>(
+ &self,
+ page_index: &ParquetColumnIndex,
+ row_group_indexes: I,
+ ) -> Result<ArrayRef>
+ where
+ I: IntoIterator<Item = &'a usize>,
+ {
+ let data_type = self.arrow_field.data_type();
+
+ let Some(parquet_column_index) = self.parquet_column_index else {
+ return Ok(self.make_null_array(data_type, row_group_indexes));
+ };
+
+ // iterator over &Index
+ let indexes = row_group_indexes
+ .into_iter()
+ .map(|rg_index| &page_index[*rg_index][parquet_column_index]);
+
+ // Get an iterator of the native index type depending on data type
+ match data_type {
Review Comment:
we need to sort out how to make this look better (perhaps by following the lead of the row group iterators that make special iterators for each underlying parquet statistics datatype and then write the relevant code for converting them all to arrow
##########
datafusion/core/tests/parquet/arrow_statistics.rs:
##########
@@ -159,11 +161,14 @@ impl TestReader {
// open the file & get the reader
let file = file.reopen().unwrap();
- ArrowReaderBuilder::try_new(file).unwrap()
+
+ // tell the reader to read the page index, if it exists
+ let options = ArrowReaderOptions::new().with_page_index(true);
+ ArrowReaderBuilder::try_new_with_options(file, options).unwrap()
}
}
-/// Defines a test case for statistics extraction
Review Comment:
This is kind of a wierd test change, but I was trying to setup a pattern where we didn't have to change all the tests at once
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] WIP Prototype DataPage extraction API [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb closed pull request #10843: WIP Prototype DataPage extraction API
URL: https://github.com/apache/datafusion/pull/10843
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] WIP Prototype DataPage extraction API [datafusion]
Posted by "marvinlanhenke (via GitHub)" <gi...@apache.org>.
marvinlanhenke commented on code in PR #10843:
URL: https://github.com/apache/datafusion/pull/10843#discussion_r1632564227
##########
datafusion/core/src/datasource/physical_plan/parquet/statistics.rs:
##########
@@ -766,10 +769,108 @@ impl<'a> StatisticsConverter<'a> {
Ok(Arc::new(UInt64Array::from_iter(null_counts)))
}
- /// Returns a null array of data_type with one element per row group
- fn make_null_array<I>(&self, data_type: &DataType, metadatas: I) -> ArrayRef
+ /// Extract the minimum values from Data page statistics
+ ///
+ /// In Parquet files, in addition to the Column Chunk level statistics
+ /// (stored for each column for each row group) there are also optional
+ /// statistics stored for each data page, part of the [Parquet Page Index].
+ /// Since a single Column Chunk is stored as one or more pages, page level statistics
+ /// can prune at a finer granularity.
+ ///
+ /// However since they are stored in a separate metadata structure
+ /// ([`Index`]) there is different code to extract them as arrow statistics
+ ///
+ /// Parameters:
+ ///
+ /// * `page_index`: The parquet page index, likely read from
+ /// [`ParquetMetadata::page_index()`]
+ ///
+ /// * row_group_indexes: The indexes of the row groups (indexes in
+ /// `page_index`) to extract the statistics from. This is an interator over `&usize` to
+ /// permit passing in `&Vec<usize>` or similar
+ ///
+ /// # Return Value
+ ///
+ /// The returned array contains 1 value for each `NativeIndex` in the underlying
+ /// `Index`es, in the same order as they appear in `metadatas`.
+ ///
+ /// For example, if there are two `Index`es in `metadatas`:
+ /// 1. the first having `3` `PageIndex` entries
+ /// 2. the second having `2` `PageIndex` entries
+ ///
+ /// The returned array would have 5 rows
+ ///
+ /// Each value is either
+ /// * the minimum value for the page
+ /// * a null value, if the statistics can not be extracted
+ ///
+ /// Note that a null value does NOT mean the min value was actually
+ /// `null` it means it the requested statistic is unknown
+ ///
+ /// # Errors
+ ///
+ /// Reasons for not being able to extract the statistics include:
+ /// * the column is not present in the parquet file
+ /// * statistics for the pages are not present in the row group
+ /// * the stored statistic value can not be converted to the requested type
+ ///
+ /// # Example
+ /// ```no_run
+ /// tood
+ /// ```
+ pub fn data_page_mins<I>(
+ &self,
+ page_index: &ParquetColumnIndex,
+ row_group_indexes: I,
+ ) -> Result<ArrayRef>
+ where
+ I: IntoIterator<Item = &'a usize>,
+ {
+ let data_type = self.arrow_field.data_type();
+
+ let Some(parquet_column_index) = self.parquet_column_index else {
+ return Ok(self.make_null_array(data_type, row_group_indexes));
+ };
+
+ // iterator over &Index
+ let indexes = row_group_indexes
+ .into_iter()
+ .map(|rg_index| &page_index[*rg_index][parquet_column_index]);
+
+ // Get an iterator of the native index type depending on data type
+ match data_type {
Review Comment:
I tried this approach, which works fine. I'd simply refactor whats mentioned in the comment - and extract data type specific iterators (like we already have for the row groups statistics). What do you think @alamb, instead of using array builder?
```Rust
pub(crate) fn min_page_statistics<'a, I: Iterator<Item = Option<&'a Index>>>(
data_type: Option<&DataType>,
iterator: I,
) -> Result<ArrayRef> {
// Extract this into data type specific iterator e.g. MinInt64PageStatisticsIterator
let iter = iterator.flat_map(|opt_index| match data_type {
Some(DataType::Int64) => match opt_index {
Some(Index::INT64(native_index)) => native_index
.indexes
.iter()
.map(|x| x.min)
.collect::<Vec<_>>(),
_ => vec![None],
},
// other data_types
_ => todo!(),
});
Ok(Arc::new(Int64Array::from_iter(iter)))
```
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] WIP Prototype DataPage extraction API [datafusion]
Posted by "marvinlanhenke (via GitHub)" <gi...@apache.org>.
marvinlanhenke commented on code in PR #10843:
URL: https://github.com/apache/datafusion/pull/10843#discussion_r1632556874
##########
datafusion/core/src/datasource/physical_plan/parquet/statistics.rs:
##########
@@ -766,10 +769,108 @@ impl<'a> StatisticsConverter<'a> {
Ok(Arc::new(UInt64Array::from_iter(null_counts)))
}
- /// Returns a null array of data_type with one element per row group
- fn make_null_array<I>(&self, data_type: &DataType, metadatas: I) -> ArrayRef
+ /// Extract the minimum values from Data page statistics
+ ///
+ /// In Parquet files, in addition to the Column Chunk level statistics
+ /// (stored for each column for each row group) there are also optional
+ /// statistics stored for each data page, part of the [Parquet Page Index].
+ /// Since a single Column Chunk is stored as one or more pages, page level statistics
+ /// can prune at a finer granularity.
+ ///
+ /// However since they are stored in a separate metadata structure
+ /// ([`Index`]) there is different code to extract them as arrow statistics
+ ///
+ /// Parameters:
+ ///
+ /// * `page_index`: The parquet page index, likely read from
+ /// [`ParquetMetadata::page_index()`]
+ ///
+ /// * row_group_indexes: The indexes of the row groups (indexes in
+ /// `page_index`) to extract the statistics from. This is an interator over `&usize` to
+ /// permit passing in `&Vec<usize>` or similar
+ ///
+ /// # Return Value
+ ///
+ /// The returned array contains 1 value for each `NativeIndex` in the underlying
+ /// `Index`es, in the same order as they appear in `metadatas`.
+ ///
+ /// For example, if there are two `Index`es in `metadatas`:
+ /// 1. the first having `3` `PageIndex` entries
+ /// 2. the second having `2` `PageIndex` entries
+ ///
+ /// The returned array would have 5 rows
+ ///
+ /// Each value is either
+ /// * the minimum value for the page
+ /// * a null value, if the statistics can not be extracted
+ ///
+ /// Note that a null value does NOT mean the min value was actually
+ /// `null` it means it the requested statistic is unknown
+ ///
+ /// # Errors
+ ///
+ /// Reasons for not being able to extract the statistics include:
+ /// * the column is not present in the parquet file
+ /// * statistics for the pages are not present in the row group
+ /// * the stored statistic value can not be converted to the requested type
+ ///
+ /// # Example
+ /// ```no_run
+ /// tood
+ /// ```
+ pub fn data_page_mins<I>(
+ &self,
+ page_index: &ParquetColumnIndex,
+ row_group_indexes: I,
Review Comment:
I'll guess one reason why we want to pass in the `row_group_indexes` is due to the iteration over the row_group_indexes from the access_plan [here](https://github.com/apache/datafusion/blob/main/datafusion/core/src/datasource/physical_plan/parquet/page_filter.rs#L168).
We cannot assume we need all indices since access_plan does [filter ](https://github.com/apache/datafusion/blob/main/datafusion/core/src/datasource/physical_plan/parquet/access_plan.rs#L299-L307) based on `should_scan()` or not.
Is this correct? If it is, then this was the missing piece in my prototype.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] WIP Prototype DataPage extraction API [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #10843:
URL: https://github.com/apache/datafusion/pull/10843#discussion_r1633288213
##########
datafusion/core/src/datasource/physical_plan/parquet/statistics.rs:
##########
@@ -766,10 +769,108 @@ impl<'a> StatisticsConverter<'a> {
Ok(Arc::new(UInt64Array::from_iter(null_counts)))
}
- /// Returns a null array of data_type with one element per row group
- fn make_null_array<I>(&self, data_type: &DataType, metadatas: I) -> ArrayRef
+ /// Extract the minimum values from Data page statistics
+ ///
+ /// In Parquet files, in addition to the Column Chunk level statistics
+ /// (stored for each column for each row group) there are also optional
+ /// statistics stored for each data page, part of the [Parquet Page Index].
+ /// Since a single Column Chunk is stored as one or more pages, page level statistics
+ /// can prune at a finer granularity.
+ ///
+ /// However since they are stored in a separate metadata structure
+ /// ([`Index`]) there is different code to extract them as arrow statistics
+ ///
+ /// Parameters:
+ ///
+ /// * `page_index`: The parquet page index, likely read from
+ /// [`ParquetMetadata::page_index()`]
+ ///
+ /// * row_group_indexes: The indexes of the row groups (indexes in
+ /// `page_index`) to extract the statistics from. This is an interator over `&usize` to
+ /// permit passing in `&Vec<usize>` or similar
+ ///
+ /// # Return Value
+ ///
+ /// The returned array contains 1 value for each `NativeIndex` in the underlying
+ /// `Index`es, in the same order as they appear in `metadatas`.
+ ///
+ /// For example, if there are two `Index`es in `metadatas`:
+ /// 1. the first having `3` `PageIndex` entries
+ /// 2. the second having `2` `PageIndex` entries
+ ///
+ /// The returned array would have 5 rows
+ ///
+ /// Each value is either
+ /// * the minimum value for the page
+ /// * a null value, if the statistics can not be extracted
+ ///
+ /// Note that a null value does NOT mean the min value was actually
+ /// `null` it means it the requested statistic is unknown
+ ///
+ /// # Errors
+ ///
+ /// Reasons for not being able to extract the statistics include:
+ /// * the column is not present in the parquet file
+ /// * statistics for the pages are not present in the row group
+ /// * the stored statistic value can not be converted to the requested type
+ ///
+ /// # Example
+ /// ```no_run
+ /// tood
+ /// ```
+ pub fn data_page_mins<I>(
+ &self,
+ page_index: &ParquetColumnIndex,
+ row_group_indexes: I,
Review Comment:
> I'll guess one reason why we want to pass in the `row_group_indexes` is due to the iteration over the row_group_indexes from the access_plan [here](https://github.com/apache/datafusion/blob/main/datafusion/core/src/datasource/physical_plan/parquet/page_filter.rs#L168).
>
> We cannot assume we need all indices since access_plan does [filter ](https://github.com/apache/datafusion/blob/main/datafusion/core/src/datasource/physical_plan/parquet/access_plan.rs#L299-L307) based on `should_scan()` or not. Is this correct? If it is, then this was the missing piece in my prototype.
Yes I think that is correct
I am still not super thrilled with this interface (mostly because it is different than `row_group_mins` etc that takes an interator directly.
However, I couldn't figure out how to make the types workout for making an iterator over Vec<Index> ...
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] WIP Prototype DataPage extraction API [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #10843:
URL: https://github.com/apache/datafusion/pull/10843#discussion_r1633286173
##########
datafusion/core/src/datasource/physical_plan/parquet/statistics.rs:
##########
@@ -766,10 +769,108 @@ impl<'a> StatisticsConverter<'a> {
Ok(Arc::new(UInt64Array::from_iter(null_counts)))
}
- /// Returns a null array of data_type with one element per row group
- fn make_null_array<I>(&self, data_type: &DataType, metadatas: I) -> ArrayRef
+ /// Extract the minimum values from Data page statistics
+ ///
+ /// In Parquet files, in addition to the Column Chunk level statistics
+ /// (stored for each column for each row group) there are also optional
+ /// statistics stored for each data page, part of the [Parquet Page Index].
+ /// Since a single Column Chunk is stored as one or more pages, page level statistics
+ /// can prune at a finer granularity.
+ ///
+ /// However since they are stored in a separate metadata structure
+ /// ([`Index`]) there is different code to extract them as arrow statistics
+ ///
+ /// Parameters:
+ ///
+ /// * `page_index`: The parquet page index, likely read from
+ /// [`ParquetMetadata::page_index()`]
+ ///
+ /// * row_group_indexes: The indexes of the row groups (indexes in
+ /// `page_index`) to extract the statistics from. This is an interator over `&usize` to
+ /// permit passing in `&Vec<usize>` or similar
+ ///
+ /// # Return Value
+ ///
+ /// The returned array contains 1 value for each `NativeIndex` in the underlying
+ /// `Index`es, in the same order as they appear in `metadatas`.
+ ///
+ /// For example, if there are two `Index`es in `metadatas`:
+ /// 1. the first having `3` `PageIndex` entries
+ /// 2. the second having `2` `PageIndex` entries
+ ///
+ /// The returned array would have 5 rows
+ ///
+ /// Each value is either
+ /// * the minimum value for the page
+ /// * a null value, if the statistics can not be extracted
+ ///
+ /// Note that a null value does NOT mean the min value was actually
+ /// `null` it means it the requested statistic is unknown
+ ///
+ /// # Errors
+ ///
+ /// Reasons for not being able to extract the statistics include:
+ /// * the column is not present in the parquet file
+ /// * statistics for the pages are not present in the row group
+ /// * the stored statistic value can not be converted to the requested type
+ ///
+ /// # Example
+ /// ```no_run
+ /// tood
+ /// ```
+ pub fn data_page_mins<I>(
+ &self,
+ page_index: &ParquetColumnIndex,
+ row_group_indexes: I,
+ ) -> Result<ArrayRef>
+ where
+ I: IntoIterator<Item = &'a usize>,
+ {
+ let data_type = self.arrow_field.data_type();
+
+ let Some(parquet_column_index) = self.parquet_column_index else {
+ return Ok(self.make_null_array(data_type, row_group_indexes));
+ };
+
+ // iterator over &Index
+ let indexes = row_group_indexes
+ .into_iter()
+ .map(|rg_index| &page_index[*rg_index][parquet_column_index]);
+
+ // Get an iterator of the native index type depending on data type
+ match data_type {
Review Comment:
I think an array builder is likely a better approach.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org