etseidl commented on code in PR #10842: URL: https://github.com/apache/arrow-rs/pull/10842#discussion_r3867072294
########## parquet/src/file/metadata/mod.rs: ########## @@ -209,70 +208,31 @@ pub(crate) use writer::ThriftMetadataWriter; /// [Page Index]: https://github.com/apache/parquet-format/blob/master/PageIndex.md /// [`ColumnIndex`]: crate::file::page_index::column_index::ColumnIndexMetaData /// [`OffsetIndex`]: crate::file::page_index::offset_index::OffsetIndexMetaData -#[derive(Debug, Clone, PartialEq)] -pub struct PageIndex { - column_indexes: Option<Vec<Vec<Option<ColumnIndexMetaData>>>>, - offset_indexes: Option<Vec<Vec<Option<OffsetIndexMetaData>>>>, -} - -impl PageIndex { - pub(crate) fn new( - column_indexes: Option<Vec<Vec<Option<ColumnIndexMetaData>>>>, - offset_indexes: Option<Vec<Vec<Option<OffsetIndexMetaData>>>>, - ) -> Self { - Self { - column_indexes, - offset_indexes, - } - } - +pub trait PageIndexProvider: Send + Sync + std::fmt::Debug { /// Returns `true` if offset index structures are present /// /// This indicates whether [`OffsetIndexMetaData`] structures were loaded or created. /// Returns `true` even if some individual columns lack offset indexes. /// /// To check if a specific column has an offset index, use [`Self::offset_index`]. - pub fn has_offset_indexes(&self) -> bool { - self.offset_indexes.is_some() - } + fn has_offset_indexes(&self) -> bool; /// Returns `true` if column index structures are present /// /// This indicates whether [`ColumnIndexMetaData`] structures were loaded or created. /// Returns `true` even if some individual columns lack column indexes. /// /// To check if a specific column has a column index, use [`Self::column_index`]. - pub fn has_column_indexes(&self) -> bool { - self.column_indexes.is_some() - } + fn has_column_indexes(&self) -> bool; /// Returns `true` if both the offset and column index structures are present /// /// This is equivalent to both [`Self::has_offset_indexes`] and [`Self::has_column_indexes`] /// returning `true`. - pub fn is_complete(&self) -> bool { + fn is_complete(&self) -> bool { self.has_column_indexes() && self.has_offset_indexes() } - /// Returns column indexes for all columns in the specified row group - /// - /// Returns `None` if: - /// - Column indexes were not loaded or are not available - /// - The row group index is out of bounds - /// - /// Returns `Some(&[Option<ColumnIndexMetaData>])` where: - /// - The slice length equals the number of columns in the row group - /// - Each element is `Some` if that column has statistics, `None` otherwise - pub fn column_indexes_for_rowgroup( Review Comment: Yes, we do. The issue is in a provider that doesn't allocate the full index, how do you return a slice? You'd have to create a vec, keep it alive somewhere, and then return a reference to that, but then that kind of kills the space savings. I added `RowGroupPageIndex` to solve some of this, but I could see also providing an iterator over the indexes in a row group. At this stage I don't want to over engineer until this actually gets used and pain points show up. -- 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]
