alamb commented on code in PR #8817:
URL: https://github.com/apache/arrow-rs/pull/8817#discussion_r2516005910
##########
parquet/src/file/metadata/writer.rs:
##########
@@ -227,22 +231,38 @@ impl<'a, W: Write> ThriftMetadataWriter<'a, W> {
None => builder.set_row_groups(row_groups),
};
- let column_indexes: Option<ParquetColumnIndex> =
column_indexes.map(|ovvi| {
- ovvi.into_iter()
- .map(|vi| {
- vi.into_iter()
- .map(|oi| oi.unwrap_or(ColumnIndexMetaData::NONE))
- .collect()
- })
- .collect()
- });
-
- // FIXME(ets): this will panic if there's a missing index.
- let offset_indexes: Option<ParquetOffsetIndex> =
offset_indexes.map(|ovvi| {
- ovvi.into_iter()
- .map(|vi| vi.into_iter().map(|oi| oi.unwrap()).collect())
- .collect()
- });
+ // test to see if all indexes for this file are empty
+ let all_none = column_indexes
+ .as_ref()
+ .is_some_and(|ci| ci.iter().all(|cii| cii.iter().all(|idx|
idx.is_none())));
+ let column_indexes: Option<ParquetColumnIndex> = if all_none {
+ None
+ } else {
+ column_indexes.map(|ovvi| {
+ ovvi.into_iter()
+ .map(|vi| {
+ vi.into_iter()
+ .map(|oi| oi.unwrap_or(ColumnIndexMetaData::NONE))
+ .collect()
+ })
+ .collect()
+ })
+ };
+
+ // test to see if all indexes for this file are empty
+ let all_none = offset_indexes
+ .as_ref()
+ .is_some_and(|oi| oi.iter().all(|oii| oii.iter().all(|idx|
idx.is_none())));
+ let offset_indexes: Option<ParquetOffsetIndex> = if all_none {
+ None
+ } else {
+ // FIXME(ets): this will panic if there's a missing index.
Review Comment:
Is this comment relevant anymore? If so maybe we should track it with a
ticket
##########
parquet/src/file/metadata/writer.rs:
##########
@@ -113,6 +113,10 @@ impl<'a, W: Write> ThriftMetadataWriter<'a, W> {
for (row_group_idx, row_group) in
self.row_groups.iter_mut().enumerate() {
for (column_idx, column_metadata) in
row_group.columns.iter_mut().enumerate() {
if let Some(column_index) =
&column_indexes[row_group_idx][column_idx] {
+ // Missing indexes may also have the placeholder
ColumnIndexMetaData::NONE
+ if matches!(column_index, ColumnIndexMetaData::NONE) {
Review Comment:
A minor style thing is that the logic about column metadata writing is now
split in two places -- here and `write_column_index` -- so someone reading
`write_column_index` may not realize that it can't be called with `NONE`
I wonder if it would be clearer if you moved this `matches` into
`write_column_index`?
You would then have to test out here if a column index was actually written
by checking bytes writtten, which might be slower I suppose 🤔
##########
parquet/src/file/metadata/writer.rs:
##########
@@ -227,22 +231,38 @@ impl<'a, W: Write> ThriftMetadataWriter<'a, W> {
None => builder.set_row_groups(row_groups),
};
- let column_indexes: Option<ParquetColumnIndex> =
column_indexes.map(|ovvi| {
- ovvi.into_iter()
- .map(|vi| {
- vi.into_iter()
- .map(|oi| oi.unwrap_or(ColumnIndexMetaData::NONE))
- .collect()
- })
- .collect()
- });
-
- // FIXME(ets): this will panic if there's a missing index.
- let offset_indexes: Option<ParquetOffsetIndex> =
offset_indexes.map(|ovvi| {
- ovvi.into_iter()
- .map(|vi| vi.into_iter().map(|oi| oi.unwrap()).collect())
- .collect()
- });
+ // test to see if all indexes for this file are empty
+ let all_none = column_indexes
Review Comment:
Minor nit would be that i would find this easier to read if it were put in a
function like `finalize_column_indexes` that could keep this already large
function smaller
The same comment applies to the offset_indexes
--
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]
