anoopj commented on code in PR #2966:
URL: https://github.com/apache/iceberg-rust/pull/2966#discussion_r3744918007
##########
crates/iceberg/src/arrow/reader/pipeline.rs:
##########
@@ -300,6 +301,55 @@ impl FileScanTaskReader {
.with_constant(RESERVED_FIELD_ID_SPEC_ID, spec_id_datum);
}
+ if task
+ .project_field_ids()
+ .contains(&RESERVED_FIELD_ID_LAST_UPDATED_SEQUENCE_NUMBER)
+ {
+ // A data file may physically carry a per-row
`_last_updated_sequence_number`
+ // column, e.g. one written by another engine such as Iceberg Java
when carrying
+ // rows forward across a rewrite. The spec requires reading such
non-null
+ // per-row values unmodified, falling back to the derived value
only where
+ // null. That per-row coalesce is not implemented yet, so rather
than silently
+ // overwrite genuine per-row values with the derived value, reject
the file
+ // loudly. Checks the full pre-projection file schema, since the
column is
+ // stripped from the projection mask.
+ let file_has_column = record_batch_stream_builder
+ .schema()
+ .fields()
+ .iter()
+ .any(|f| {
+ f.metadata()
+ .get(PARQUET_FIELD_ID_META_KEY)
+ .and_then(|id| id.parse::<i32>().ok())
+ == Some(RESERVED_FIELD_ID_LAST_UPDATED_SEQUENCE_NUMBER)
+ });
+ if file_has_column {
+ return Err(Error::new(
+ ErrorKind::FeatureUnsupported,
+ "Reading a physically-stored _last_updated_sequence_number
column is \
+ not yet supported; only the derived
(data-sequence-number) value is \
+ implemented",
+ ));
+ }
+
+ // Derive the column from the data file's sequence number, gated on
+ // `first_row_id`: per the spec's Row Lineage read rules, a data
file with a
+ // non-null `first_row_id` inherits
`_last_updated_sequence_number` from its
+ // data sequence number, while a file with a null `first_row_id`
(v1/v2, or a
+ // pre-upgrade v3 snapshot) produces null.
+ record_batch_transformer_builder = match (task.first_row_id,
task.data_sequence_number)
+ {
+ (Some(_), Some(seq)) =>
record_batch_transformer_builder.with_constant(
+ RESERVED_FIELD_ID_LAST_UPDATED_SEQUENCE_NUMBER,
+ Datum::long(seq),
+ ),
+ // (None, _) is the null gate. (Some, None), first_row_id
present but no
+ // data sequence number (a malformed manifest), also yields
null.
+ _ => record_batch_transformer_builder
Review Comment:
Agreed. Changed that arm to return `DataInvalid` and updated the test.
--
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]
