alamb commented on code in PR #10449:
URL: https://github.com/apache/arrow-rs/pull/10449#discussion_r3714781081
##########
parquet/src/arrow/array_reader/row_number.rs:
##########
@@ -47,14 +47,30 @@ impl RowNumberReader {
// This is O(M) where M is the total number of row groups in the file
let mut ordinal_to_offset: HashMap<i32, i64> = HashMap::new();
let mut first_row_index: i64 = 0;
+ let mut missing_ordinals: usize = 0;
for rg in parquet_metadata.row_groups() {
if let Some(ordinal) = rg.ordinal() {
ordinal_to_offset.insert(ordinal, first_row_index);
+ } else {
+ missing_ordinals += 1;
}
first_row_index += rg.num_rows();
}
+ // Mixed ordinals: refuse to compute row numbers for the whole file,
+ // even if every *selected* row group carries an ordinal. Otherwise the
+ // same file would yield row numbers or an error depending on which row
+ // groups a query's pruning happens to select.
+ if missing_ordinals > 0 && !ordinal_to_offset.is_empty() {
Review Comment:
👍
##########
parquet/src/file/metadata/thrift/mod.rs:
##########
@@ -2035,4 +2014,97 @@ pub(crate) mod tests {
.expect_err("malformed bool field should return an error");
assert_malformed_bool_error(err);
}
+
+ /// Round-trip [`crate::file::metadata::ParquetMetaData`] with the given
+ /// per-row-group ordinals through thrift encode → decode, returning the
+ /// decoded ordinals. Exercises `ensure_row_group_ordinals`.
+ fn roundtrip_rg_ordinals(ordinals: &[Option<i32>]) -> Vec<Option<i32>> {
+ use crate::file::metadata::ParquetMetaDataWriter;
+ use crate::file::metadata::{FileMetaData, ParquetMetaData,
ParquetMetaDataReader};
+ use crate::schema::types::Type as SchemaType;
+
+ let field = SchemaType::primitive_type_builder("c",
PhysicalType::INT32)
+ .build()
+ .unwrap();
+ let schema = SchemaType::group_type_builder("schema")
+ .with_fields(vec![Arc::new(field)])
+ .build()
+ .unwrap();
+ let schema_descr = Arc::new(SchemaDescriptor::new(Arc::new(schema)));
+
+ let row_groups = ordinals
+ .iter()
+ .map(|ordinal| {
+ let columns = schema_descr
+ .columns()
+ .iter()
+ .map(|col|
ColumnChunkMetaData::builder(col.clone()).build().unwrap())
+ .collect();
+ let mut builder =
+
crate::file::metadata::RowGroupMetaData::builder(schema_descr.clone())
+ .set_num_rows(10)
+ .set_total_byte_size(100)
+ .set_column_metadata(columns);
+ if let Some(ordinal) = ordinal {
+ builder = builder.set_ordinal(*ordinal);
+ }
+ builder.build().unwrap()
+ })
+ .collect();
+
+ let file_metadata = FileMetaData::new(
+ 1,
+ 10 * ordinals.len() as i64,
+ None,
+ None,
+ schema_descr,
+ None,
+ );
+ let metadata = ParquetMetaData::new(file_metadata, row_groups);
+
+ let mut buffer = Vec::new();
+ ParquetMetaDataWriter::new(&mut buffer, &metadata)
+ .finish()
+ .unwrap();
+ // strip the 8-byte footer tail (length + magic)
+ let decoded =
ParquetMetaDataReader::decode_metadata(&buffer[..buffer.len() - 8]).unwrap();
+ decoded.row_groups().iter().map(|rg| rg.ordinal()).collect()
+ }
+
+ /// All row groups carry ordinals: honored as written, even when they do
+ /// not match file position.
+ #[test]
+ fn ordinals_all_present_are_honored() {
+ assert_eq!(
+ roundtrip_rg_ordinals(&[Some(5), Some(1), Some(3)]),
+ vec![Some(5), Some(1), Some(3)],
+ );
+ }
+
+ /// No row group carries an ordinal: sequential-filled at decode time so
+ /// downstream consumers behave identically on fresh vs reused metadata.
+ #[test]
+ fn ordinals_none_present_are_sequentially_filled() {
+ assert_eq!(
+ roundtrip_rg_ordinals(&[None, None, None]),
+ vec![Some(0), Some(1), Some(2)],
+ );
+ }
+
+ /// Mixed ordinals (spec-valid; produced by e.g. Go parquet writers):
Review Comment:
this is clever
##########
parquet/src/arrow/arrow_reader/mod.rs:
##########
@@ -5649,6 +5649,85 @@ pub(crate) mod tests {
Ok(())
}
+ /// A file with *mixed* row-group ordinal metadata (spec-valid — the
+ /// `RowGroup.ordinal` thrift field is optional; Go parquet writers emit
+ /// such files) must read fine without row numbers, and must fail
+ /// deterministically with them — even when every *selected* row group
+ /// carries an ordinal. See #10381.
Review Comment:
i recommend making this an actual github url link for east
clicking/copy-pasting
##########
parquet/src/file/metadata/thrift/encryption.rs:
##########
@@ -163,10 +163,21 @@ fn row_group_from_encrypted_thrift(
}
};
+ // The ordinal is part of the AAD for encrypted column metadata.
+ // It can be missing here only for files with *mixed* row-group
+ // ordinals, which decode leaves untouched — fail cleanly rather
+ // than panic (see `ensure_row_group_ordinals`).
+ let rg_ordinal = rg.ordinal.ok_or_else(|| {
Review Comment:
Agree this is better than the panic the old code would do
--
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]
