adriangb commented on code in PR #25335:
URL: https://github.com/apache/datafusion/pull/25335#discussion_r4041757484
##########
datafusion/common/src/metadata.rs:
##########
@@ -131,7 +125,7 @@ pub fn check_metadata_with_storage_equal(
/// renderings.
pub fn format_type_and_metadata(
data_type: &DataType,
- metadata: Option<&std::collections::HashMap<String, String>>,
+ metadata: Option<&Metadata>,
Review Comment:
Nice
##########
datafusion/datasource-parquet/src/metadata.rs:
##########
@@ -1049,8 +1058,10 @@ impl FileMetadata for CachedParquetMetaData {
}
fn extra_info(&self) -> HashMap<String, String> {
- let page_index =
- self.0.column_index().is_some() && self.0.offset_index().is_some();
+ let page_index = self
+ .0
+ .page_index()
+ .is_some_and(|page_index| page_index.is_complete());
Review Comment:
Much nicer API.
##########
datafusion/core/src/datasource/file_format/parquet.rs:
##########
@@ -1125,34 +1123,28 @@ mod tests {
.await?
.metadata()
.clone();
- check_page_index_validation(builder.column_index(),
builder.offset_index());
+ check_page_index_validation(builder.page_index());
Ok(())
}
- fn check_page_index_validation(
- page_index: Option<&ParquetColumnIndex>,
- offset_index: Option<&ParquetOffsetIndex>,
- ) {
- assert!(page_index.is_some());
- assert!(offset_index.is_some());
-
+ fn check_page_index_validation(page_index: Option<&Arc<dyn
PageIndexProvider>>) {
let page_index = page_index.unwrap();
- let offset_index = offset_index.unwrap();
-
- // there is only one row group in one file.
- assert_eq!(page_index.len(), 1);
- assert_eq!(offset_index.len(), 1);
- let page_index = page_index.first().unwrap();
- let offset_index = offset_index.first().unwrap();
-
- // 13 col in one row group
- assert_eq!(page_index.len(), 13);
- assert_eq!(offset_index.len(), 13);
+ assert!(page_index.is_complete());
+
+ // there is only one row group in one file, with 13 columns.
+ // All columns have an offset index; all except column 10 also have a
+ // column index.
+ for col in 0..13 {
+ assert_eq!(page_index.column_index(0, col).is_some(), col != 10);
+ assert!(page_index.offset_index(0, col).is_some());
+ }
+ assert!(page_index.column_index(1, 0).is_none());
+ assert!(page_index.offset_index(1, 0).is_none());
Review Comment:
Seems like this is testing more in-depth, not sure if needed but seems okay
##########
datafusion/datasource-parquet/src/page_filter.rs:
##########
@@ -708,14 +710,13 @@ fn prune_pages_in_one_row_group(
Some((RowSelection::from(vec), values))
}
-/// Implement [`PruningStatistics`] for one column's PageIndex (column_index +
offset_index)
+/// Implement [`PruningStatistics`] for one column's [`PageIndexProvider`]
#[derive(Debug)]
struct PagesPruningStatistics<'a> {
row_group_index: usize,
row_group_metadatas: &'a [RowGroupMetaData],
converter: StatisticsConverter<'a>,
- column_index: &'a ParquetColumnIndex,
- offset_index: &'a ParquetOffsetIndex,
+ page_index: &'a dyn PageIndexProvider,
Review Comment:
Although... [we may end up backtracking on
it](https://github.com/apache/arrow-rs/issues/7582#issuecomment-5702793840) (or
just having 1 implementation)
##########
datafusion/datasource-parquet/src/metadata.rs:
##########
@@ -80,7 +80,12 @@ pub(crate) fn has_untrusted_min_max_order(
parquet_column_index: usize,
) -> bool {
let column = parquet_schema.column(parquet_column_index);
- if column.sort_order() == SortOrder::UNDEFINED {
+ // As of arrow 60, INT96 columns report `SortOrder::INT96_TIMESTAMP`
+ // rather than `UNDEFINED`; keep treating their min/max as untrusted.
+ if matches!(
+ column.sort_order(),
+ SortOrder::UNDEFINED | SortOrder::INT96_TIMESTAMP
Review Comment:
as in revisit the arrow change or revisit if we treat
`SortOrder::INT96_TIMESTAMP` as untrusted? i'd guess with the sort order
correctness changes @etseidl has driven we can now trust it, but not too sure.
--
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]
