zhuqi-lucas commented on code in PR #10449:
URL: https://github.com/apache/arrow-rs/pull/10449#discussion_r3718899201
##########
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:
Done — switched to the full
`<https://github.com/apache/arrow-rs/issues/10381>` link.
##########
parquet/src/file/metadata/thrift/mod.rs:
##########
@@ -932,56 +930,37 @@ pub(crate) fn parquet_metadata_from_bytes(
Ok(ParquetMetaData::new(fmd, row_groups))
}
-/// Assign [`RowGroupMetaData::ordinal`] if it is missing.
-#[derive(Debug, Default)]
-pub(crate) struct OrdinalAssigner {
- first_has_ordinal: Option<bool>,
-}
-
-impl OrdinalAssigner {
- fn new() -> Self {
- Default::default()
+/// Ensure [`RowGroupMetaData::ordinal`] is usable after decode without
+/// rejecting spec-valid files (`RowGroup.ordinal` is optional in the
+/// parquet-format Thrift definition, with no uniformity requirement):
+///
+/// - **All row groups carry ordinals** → honor them as written.
+/// - **No row group carries an ordinal** → assign each row group its
+/// position in the file. This happens unconditionally (not only when a
+/// consumer needs it) so downstream users of the ordinal — the row
+/// number virtual column, encryption chunk-key lookup — behave the same
+/// whether the metadata was decoded fresh or reused from a prior read.
+/// - **Mixed** → leave the metadata untouched. Positional backfill could
+/// disagree with the ordinals that are present, and a partial backfill
+/// would make row-number results depend on which row groups a query
+/// happens to select. Consumers that require complete ordinals fail
+/// deterministically instead (see `RowNumberReader::try_new`); plain
+/// reads that never touch ordinals succeed.
+fn ensure_row_group_ordinals(row_groups: &mut [RowGroupMetaData]) ->
Result<()> {
+ if row_groups.iter().all(|rg| rg.ordinal.is_some()) {
+ return Ok(());
}
-
- /// Sets [`RowGroupMetaData::ordinal`] if it is missing.
- ///
- /// # Arguments
- /// - actual_ordinal: The ordinal (index) of the row group being processed
- /// in the file metadata.
- /// - rg: The [`RowGroupMetaData`] to potentially modify.
- ///
- /// Ensures:
- /// 1. If the first row group has an ordinal, all subsequent row groups
must
- /// also have ordinals.
- /// 2. If the first row group does NOT have an ordinal, all subsequent row
- /// groups must also not have ordinals.
- fn ensure(
- &mut self,
- actual_ordinal: i32,
- mut rg: RowGroupMetaData,
- ) -> Result<RowGroupMetaData> {
- let rg_has_ordinal = rg.ordinal.is_some();
-
- // Only set first_has_ordinal if it's None (first row group that
arrives)
- if self.first_has_ordinal.is_none() {
- self.first_has_ordinal = Some(rg_has_ordinal);
- }
-
- // assign ordinal if missing and consistent with first row group
- let first_has_ordinal = self.first_has_ordinal.unwrap();
- if !first_has_ordinal && !rg_has_ordinal {
- rg.ordinal = Some(actual_ordinal);
- } else if first_has_ordinal != rg_has_ordinal {
- return Err(general_err!(
- "Inconsistent ordinal assignment: first_has_ordinal is set to \
- {} but row-group with actual ordinal {} has rg_has_ordinal set
to {}",
- first_has_ordinal,
- actual_ordinal,
- rg_has_ordinal
- ));
- }
- Ok(rg)
+ if row_groups.iter().any(|rg| rg.ordinal.is_some()) {
+ // Mixed: leave as-is.
Review Comment:
Good catch — both the "all ordinals present" and "mixed" cases leave the
metadata untouched, so I collapsed them into a single `any(is_some)` early
return; only the "no ordinals at all" case falls through to the positional
backfill.
--
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]
