CTTY commented on code in PR #2695:
URL: https://github.com/apache/iceberg-rust/pull/2695#discussion_r3574204042
##########
crates/iceberg/src/arrow/record_batch_transformer.rs:
##########
@@ -481,16 +461,39 @@ impl RecordBatchTransformer {
projected_iceberg_field_ids
.iter()
.map(|field_id| {
- // Check if this is a constant field (metadata/virtual or
identity-partitioned)
- // Constant fields always use their pre-computed constant
values, regardless of whether
- // they exist in the Parquet file. This is per Iceberg spec
rule #1: partition metadata
- // is authoritative and should be preferred over file data.
+ // Check if this is a constant field (metadata/virtual or
identity-partitioned).
+ //
+ // Metadata/virtual fields (like _file, _spec_id) never exist
in the data file,
+ // so they always use their pre-computed constant value.
+ //
+ // For identity-partitioned fields, the Iceberg spec's "Column
Projection" rules
+ // only apply to "field ids which are not present in a data
file". When the column
+ // IS present in the Parquet file, it must be read from the
file; the partition
+ // metadata constant is only a fallback for when the column is
absent (e.g. add_files).
Review Comment:
Good catch
##########
crates/iceberg/src/arrow/record_batch_transformer.rs:
##########
@@ -359,49 +359,29 @@ impl RecordBatchTransformer {
let fields: Result<Vec<_>> = projected_iceberg_field_ids
.iter()
.map(|field_id| {
- // Check if this is a constant field
- if constant_fields.contains_key(field_id) {
- // For metadata/virtual fields (like _file), get name from
metadata_columns
- // For partition fields, get name from schema (they exist
in schema)
- if let Ok(iceberg_field) = get_metadata_field(*field_id) {
- // This is a metadata/virtual field - convert Iceberg
field to Arrow
- let datum =
constant_fields.get(field_id).ok_or(Error::new(
- ErrorKind::Unexpected,
- "constant field not found",
- ))?;
- let arrow_type = datum_to_arrow_type_with_ree(datum);
- let arrow_field =
- Field::new(&iceberg_field.name, arrow_type,
!iceberg_field.required)
- .with_metadata(HashMap::from([(
- PARQUET_FIELD_ID_META_KEY.to_string(),
- iceberg_field.id.to_string(),
- )]));
- Ok(Arc::new(arrow_field))
- } else {
- // This is a partition constant field (exists in
schema but uses constant value)
- let field = &field_id_to_mapped_schema_map
- .get(field_id)
- .ok_or(Error::new(ErrorKind::Unexpected, "field
not found"))?
- .0;
- let datum =
constant_fields.get(field_id).ok_or(Error::new(
- ErrorKind::Unexpected,
- "constant field not found",
- ))?;
- let arrow_type = datum_to_arrow_type_with_ree(datum);
- // Use the type from constant_fields (REE for
constants)
- let constant_field =
- Field::new(field.name(), arrow_type,
field.is_nullable())
- .with_metadata(field.metadata().clone());
- Ok(Arc::new(constant_field))
- }
- } else {
- // Regular field - use schema as-is
- Ok(field_id_to_mapped_schema_map
- .get(field_id)
- .ok_or(Error::new(ErrorKind::Unexpected, "field not
found"))?
- .0
- .clone())
+ // Metadata/virtual fields (like _file, _spec_id) don't exist
in the table
+ // schema, so build their Arrow field from the metadata column
definition and
+ // the pre-computed constant's type.
+ if let Some(datum) = constant_fields.get(field_id)
+ && let Ok(iceberg_field) = get_metadata_field(*field_id)
+ {
+ let arrow_type = datum_to_arrow_type_with_ree(datum);
+ let arrow_field =
+ Field::new(&iceberg_field.name, arrow_type,
!iceberg_field.required)
+ .with_metadata(HashMap::from([(
+ PARQUET_FIELD_ID_META_KEY.to_string(),
+ iceberg_field.id.to_string(),
+ )]));
+ return Ok(Arc::new(arrow_field));
}
+
+ // Regular fields and identity-partitioned constant fields
both exist in the
+ // table schema, so use the mapped Arrow field as-is.
+ Ok(field_id_to_mapped_schema_map
+ .get(field_id)
+ .ok_or(Error::new(ErrorKind::Unexpected, "field not
found"))?
+ .0
+ .clone())
Review Comment:
Nice refactoring!
##########
crates/iceberg/src/arrow/reader/pipeline.rs:
##########
@@ -248,6 +250,15 @@ impl FileScanTaskReader {
record_batch_transformer_builder.with_constant(RESERVED_FIELD_ID_FILE,
file_datum);
}
+ if task
+ .project_field_ids()
+ .contains(&RESERVED_FIELD_ID_SPEC_ID)
+ {
+ let spec_id_datum =
Datum::int(task.partition_spec.clone().unwrap().spec_id());
Review Comment:
could the partition spec be none here? what is the expected behavior when
task.partition_spec is none?
--
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]
