alamb commented on code in PR #10653: URL: https://github.com/apache/arrow-rs/pull/10653#discussion_r3769801874
########## parquet/src/file/metadata/mod.rs: ########## @@ -141,29 +141,31 @@ pub(crate) use writer::ThriftMetadataWriter; /// documentation]. Each [`ColumnIndex`] holds statistics about all the pages in a /// particular column chunk. /// -/// `column_index[row_group_number][column_number]` holds the +/// `column_index[row_group_number][column_number]` holds the optional /// [`ColumnIndex`] corresponding to column `column_number` of row group -/// `row_group_number`. +/// `row_group_number`. This will be `None` if no index is present for the given +/// column chunk. /// /// For example `column_index[2][3]` holds the [`ColumnIndex`] for the fourth /// column in the third row group of the parquet file. /// /// [PageIndex documentation]: https://github.com/apache/parquet-format/blob/master/PageIndex.md /// [`ColumnIndex`]: crate::file::page_index::column_index::ColumnIndexMetaData -pub type ParquetColumnIndex = Vec<Vec<ColumnIndexMetaData>>; +pub type ParquetColumnIndex = Vec<Vec<Option<ColumnIndexMetaData>>>; Review Comment: I have always found this structure to be very confusing (as it is a Vec of Vecs). Adding Option makes it even more confusing in my mind. What would you think about at least encapsulating the PageIndex into a structure of its own (rather than two parallel structure)? ```rust struct PageIndex { column_indexes: Vec<Vec<Option<ColumnIndexMetaData>>>, offset_indexes: Vec<Vec<Option<OffsetIndexMetaData>>>, } ``` 🤔 Then we could add accessors like ```rust if let Some(offset_index) = page_index.offset_index(rg_idx) { // use offset index for rg_idx } ``` That might also allow us to tweak the internal representation of these indexes to support options, etc without breaking the structure again ########## parquet/src/file/metadata/mod.rs: ########## @@ -141,29 +141,31 @@ pub(crate) use writer::ThriftMetadataWriter; /// documentation]. Each [`ColumnIndex`] holds statistics about all the pages in a /// particular column chunk. /// -/// `column_index[row_group_number][column_number]` holds the +/// `column_index[row_group_number][column_number]` holds the optional /// [`ColumnIndex`] corresponding to column `column_number` of row group -/// `row_group_number`. +/// `row_group_number`. This will be `None` if no index is present for the given +/// column chunk. /// /// For example `column_index[2][3]` holds the [`ColumnIndex`] for the fourth /// column in the third row group of the parquet file. /// /// [PageIndex documentation]: https://github.com/apache/parquet-format/blob/master/PageIndex.md /// [`ColumnIndex`]: crate::file::page_index::column_index::ColumnIndexMetaData -pub type ParquetColumnIndex = Vec<Vec<ColumnIndexMetaData>>; +pub type ParquetColumnIndex = Vec<Vec<Option<ColumnIndexMetaData>>>; -/// [`OffsetIndexMetaData`] for each data page of each row group of each column +/// [`OffsetIndexMetaData`] for each column chunk of each row group /// /// This structure is the parsed representation of the [`OffsetIndex`] from the /// Parquet file footer, as described in the Parquet [PageIndex documentation]. /// /// `offset_index[row_group_number][column_number]` holds -/// the [`OffsetIndexMetaData`] corresponding to column -/// `column_number`of row group `row_group_number`. +/// the optional [`OffsetIndexMetaData`] corresponding to column +/// `column_number`of row group `row_group_number`. This will be `None` if no index +/// is present for the given column chunk. /// /// [PageIndex documentation]: https://github.com/apache/parquet-format/blob/master/PageIndex.md /// [`OffsetIndex`]: https://github.com/apache/parquet-format/blob/master/PageIndex.md -pub type ParquetOffsetIndex = Vec<Vec<OffsetIndexMetaData>>; +pub type ParquetOffsetIndex = Vec<Vec<Option<OffsetIndexMetaData>>>; Review Comment: If we are going to change the page index representation (and force a breaking change downstream on the users) I wonder if we can think bigger than just adding an Option here and making it align with the parquet-format names For example what do you think about making it a struct so that we have a better chance of evolving it over time (and make it easier to document)? For example: https://github.com/apache/parquet-format/blob/2076361bb64e2de9ca6a8d06eda025a6fa4e9df6/src/main/thrift/parquet.thrift#L1261 ```rust struct ParquetOffsetIndex { page_locations: Vec<ParquetPageLocation>, unencoded_byte_array_data_bytes: Option<Vec<i64>, } ``` And https://github.com/apache/parquet-format/blob/2076361bb64e2de9ca6a8d06eda025a6fa4e9df6/src/main/thrift/parquet.thrift#L1236 ```rust struct ParquetPageLocation { offset: i64, compressed_page_size: i32, first_row_index: i64, } ``` ########## parquet/src/file/metadata/mod.rs: ########## @@ -141,29 +141,31 @@ pub(crate) use writer::ThriftMetadataWriter; /// documentation]. Each [`ColumnIndex`] holds statistics about all the pages in a /// particular column chunk. /// -/// `column_index[row_group_number][column_number]` holds the +/// `column_index[row_group_number][column_number]` holds the optional /// [`ColumnIndex`] corresponding to column `column_number` of row group -/// `row_group_number`. +/// `row_group_number`. This will be `None` if no index is present for the given +/// column chunk. /// /// For example `column_index[2][3]` holds the [`ColumnIndex`] for the fourth /// column in the third row group of the parquet file. /// /// [PageIndex documentation]: https://github.com/apache/parquet-format/blob/master/PageIndex.md /// [`ColumnIndex`]: crate::file::page_index::column_index::ColumnIndexMetaData -pub type ParquetColumnIndex = Vec<Vec<ColumnIndexMetaData>>; +pub type ParquetColumnIndex = Vec<Vec<Option<ColumnIndexMetaData>>>; -/// [`OffsetIndexMetaData`] for each data page of each row group of each column +/// [`OffsetIndexMetaData`] for each column chunk of each row group /// /// This structure is the parsed representation of the [`OffsetIndex`] from the /// Parquet file footer, as described in the Parquet [PageIndex documentation]. /// /// `offset_index[row_group_number][column_number]` holds -/// the [`OffsetIndexMetaData`] corresponding to column -/// `column_number`of row group `row_group_number`. +/// the optional [`OffsetIndexMetaData`] corresponding to column +/// `column_number`of row group `row_group_number`. This will be `None` if no index +/// is present for the given column chunk. /// /// [PageIndex documentation]: https://github.com/apache/parquet-format/blob/master/PageIndex.md /// [`OffsetIndex`]: https://github.com/apache/parquet-format/blob/master/PageIndex.md -pub type ParquetOffsetIndex = Vec<Vec<OffsetIndexMetaData>>; +pub type ParquetOffsetIndex = Vec<Vec<Option<OffsetIndexMetaData>>>; Review Comment: We could also just do something slightly more encapsulated rather than a typedef ```rust struct ParquetOffsetIndex { inner: Vec<Vec<Option<OffsetIndexMetaData>>>; } ``` 🤔 -- 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]
