advancedxy commented on code in PR #2668:
URL: https://github.com/apache/iceberg-rust/pull/2668#discussion_r3628177500
##########
crates/iceberg/src/scan/mod.rs:
##########
@@ -300,6 +303,20 @@ impl<'a> TableScanBuilder<'a> {
.transpose()?
.map(Arc::new);
+ // Compute unified partition type if _partition is projected
+ let unified_partition_type = if
field_ids.contains(&RESERVED_FIELD_ID_PARTITION) {
+ let upt = compute_unified_partition_type(
Review Comment:
nit: `upt` seems a bit of unusual in the iceberg-rust repo. how about simply
call it `partition_type` or `unified_type`?
##########
crates/iceberg/src/arrow/record_batch_transformer.rs:
##########
@@ -186,16 +211,82 @@ enum SchemaComparison {
/// Builder for RecordBatchTransformer to improve ergonomics when constructing
with optional parameters.
///
-/// Constant fields are pre-computed for both virtual/metadata fields (like
_file) and
-/// identity-partitioned fields to avoid duplicate work during batch
processing.
+/// All per-file constants (scalar metadata like `_file`, identity partition
values,
+/// and the `_partition` struct) are stored in a single `metadata_columns` map
keyed by
+/// field_id. This unified representation (via [`MetadataColumnSource`]) means
the
+/// transformer handles all constant columns through one code path.
#[derive(Debug)]
pub(crate) struct RecordBatchTransformerBuilder {
snapshot_schema: Arc<IcebergSchema>,
projected_iceberg_field_ids: Vec<i32>,
- constant_fields: HashMap<i32, Datum>,
+ metadata_columns: HashMap<i32, MetadataColumnSource>,
Review Comment:
hmm, I'd prefer to use constant fields here, which aligns with java's
concept. Although, the constant fields are almost always metadata columns. But
the identity partition source field is also injected into the constant map.
##########
crates/iceberg/src/arrow/record_batch_transformer.rs:
##########
@@ -376,39 +473,54 @@ impl RecordBatchTransformer {
let fields: Result<Vec<_>> = projected_iceberg_field_ids
.iter()
.map(|field_id| {
- // 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(),
- )]));
- Ok(Arc::new(arrow_field))
- } else if virtual_fields.contains(field_id) {
+ match metadata_columns.get(field_id) {
+ Some(MetadataColumnSource::Struct(pc)) => {
+ let struct_type =
DataType::Struct(pc.fields().clone());
+ let nullable = pc.fields().is_empty();
+ let arrow_field = field_with_id(
+ RESERVED_COL_NAME_PARTITION,
Review Comment:
I think this branch specialized/assumed the `MetadataColumnSource::Struct`
must come from the _partition field, which is true for current code but might
change when the spec evolves. It would be better to check the field_id is
indeed the partition col field first.
##########
crates/iceberg/src/arrow/record_batch_transformer.rs:
##########
@@ -186,16 +211,82 @@ enum SchemaComparison {
/// Builder for RecordBatchTransformer to improve ergonomics when constructing
with optional parameters.
///
-/// Constant fields are pre-computed for both virtual/metadata fields (like
_file) and
-/// identity-partitioned fields to avoid duplicate work during batch
processing.
+/// All per-file constants (scalar metadata like `_file`, identity partition
values,
+/// and the `_partition` struct) are stored in a single `metadata_columns` map
keyed by
+/// field_id. This unified representation (via [`MetadataColumnSource`]) means
the
+/// transformer handles all constant columns through one code path.
#[derive(Debug)]
pub(crate) struct RecordBatchTransformerBuilder {
snapshot_schema: Arc<IcebergSchema>,
projected_iceberg_field_ids: Vec<i32>,
- constant_fields: HashMap<i32, Datum>,
+ metadata_columns: HashMap<i32, MetadataColumnSource>,
virtual_fields: HashSet<i32>,
}
+/// Unified source for metadata/virtual column constants.
+///
+/// Both scalar metadata columns (like `_file`) and the struct `_partition`
column are
+/// per-file constants synthesized during scan. This enum unifies both
representations
+/// so the transformer can handle them through a single `metadata_columns` map
keyed
+/// by field_id, replacing the previous split between `constant_fields:
HashMap<i32, Datum>`
+/// and `partition_column: Option<PartitionColumnConstant>`.
+///
+/// # Design for #2699
+///
+/// When `_pos` is implemented (via arrow-rs RowNumber — a real source column
passed through,
+/// not synthesized), it will NOT use this enum. `_pos` is not a constant and
belongs in the
+/// virtual_fields / pass-through axis. This enum is exclusively for
constant-per-file values.
+///
+/// The `ColumnSource` enum's `Add` and `AddStructConstant` variants map 1:1
to this enum's
+/// variants during transform generation. #2699 may collapse
`ColumnSource::Add` and
+/// `ColumnSource::AddStructConstant` into a single `ColumnSource::AddMetadata
{ source: MetadataColumnSource }`
+/// variant, but that refactor can happen independently.
+#[derive(Debug, Clone, PartialEq)]
+pub enum MetadataColumnSource {
+ /// A scalar constant (e.g., `_file` path, `_spec_id`, identity partition
values).
+ /// The Datum carries both the Iceberg type and the value.
+ Scalar(Datum),
+ /// A struct constant (the `_partition` column). Each child is a primitive
constant
+ /// or null (for partition evolution gaps).
+ Struct(PartitionColumnConstant),
+}
+
+/// Pre-computed data for the _partition struct constant.
+#[derive(Debug, Clone, PartialEq)]
+pub struct PartitionColumnConstant {
Review Comment:
this could be called as `StructConstant`? It could cover other struct
constant if needed, not just the partition column.
--
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]
