anoopj commented on code in PR #2985:
URL: https://github.com/apache/iceberg-rust/pull/2985#discussion_r3763806744
##########
crates/iceberg/src/arrow/reader/pipeline.rs:
##########
@@ -250,6 +252,42 @@ impl FileScanTaskReader {
let mut record_batch_stream_builder =
ParquetRecordBatchStreamBuilder::new_with_metadata(parquet_file_reader,
arrow_metadata);
+ // Whether the file physically carries the
`_last_updated_sequence_number` column
+ // (some engines, e.g. Iceberg Java on rewrite, write it per-row),
resolved by its
+ // embedded field id against the Parquet schema.
+ let project_last_updated_seq = task
+ .project_field_ids()
+ .contains(&RESERVED_FIELD_ID_LAST_UPDATED_SEQUENCE_NUMBER);
+
+ // Parquet leaf index of the physical column, if present by embedded
field id.
+ // `build_field_id_map` is all-or-nothing (`None` if any column lacks
an id), so a
+ // file mixing id-bearing and id-less columns is rejected below rather
than
+ // coalesced -- safe, and consistent with the rest of the reader.
+ let phys_last_updated_seq_leaf = if project_last_updated_seq {
+
build_field_id_map(record_batch_stream_builder.parquet_schema())?.and_then(|m| {
+ m.get(&RESERVED_FIELD_ID_LAST_UPDATED_SEQUENCE_NUMBER)
+ .copied()
+ })
+ } else {
+ None
+ };
+
+ // Present by name but not by the embedded id (only meaningful when no
by-id column
+ // was found). An unthreadable shape we reject rather than coalesce
incorrectly.
+ let last_updated_seq_present_by_name_only = project_last_updated_seq
+ && phys_last_updated_seq_leaf.is_none()
+ && record_batch_stream_builder
+ .schema()
+ .fields()
+ .iter()
+ .any(|f| f.name() ==
RESERVED_COL_NAME_LAST_UPDATED_SEQUENCE_NUMBER);
+
+ // We read + coalesce the physical column only when it is present by
id AND the file
+ // is row-lineage-bearing (first_row_id set) with a data sequence
number to fall
+ // back to. When first_row_id is null the column is nulled, so we must
not read it.
+ let coalesce_last_updated_seq_leaf = phys_last_updated_seq_leaf
+ .filter(|_| task.first_row_id.is_some() &&
task.data_sequence_number.is_some());
Review Comment:
Kept the behavior, documented it as a deliberate Java-parity choice. Passing
values through would diverge from Java because a file with no row lineage has
no meaningful per-row sequence numbers, so I'd rather keep them consistent.
One comment about the `data_sequence_number=None` half: that one isn't
silent. It lands in the (Some, None) arm which returns DataInvalid. Added a
test for it.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
