laskoviymishka commented on code in PR #3437:
URL: https://github.com/apache/iceberg-rust/pull/3437#discussion_r4326450440
##########
crates/iceberg/src/arrow/record_batch_transformer.rs:
##########
@@ -270,19 +270,39 @@ impl PromotePlan {
}),
(DataType::Map(source_entries, _), DataType::Map(target_entries,
sorted)) => {
match (source_entries.data_type(), target_entries.data_type())
{
- (DataType::Struct(source_fields),
DataType::Struct(target_fields)) => {
+ // Key and value match by position, as in iceberg-java's
map readers.
Review Comment:
Follow-up: this now matches key and value by position for files with ids
too, not only id-less ones. That's safe, since the Parquet `key_value` group is
always key then value, but the comment reads as if it were only the id-less
fallback. I'd say it applies with or without ids, and add a test with an
id-bearing map whose value struct gained a field, to pin that ids still resolve
one level down through `build`.
##########
crates/iceberg/src/arrow/reader/projection.rs:
##########
@@ -1807,16 +1807,9 @@ message schema {
let person_array = batch.column(1).as_struct();
assert_eq!(person_array.num_columns(), 2);
-
- let name_array = person_array.column(0).as_string::<i32>();
- assert_eq!(name_array.value(0), "Alice");
- assert_eq!(name_array.value(1), "Bob");
-
- let age_array = person_array
- .column(1)
- .as_primitive::<arrow_array::types::Int32Type>();
- assert_eq!(age_array.value(0), 30);
- assert_eq!(age_array.value(1), 25);
+ assert_eq!(person_array.null_count(), 0);
Review Comment:
Follow-up: these would also pass on a zero-length struct. I'd compare
against an expected `StructArray` the way
`promote_struct_without_source_field_ids_reads_null` does (two rows, struct
non-null, `new_null_array` children), so the end-to-end path also pins the kept
struct validity.
##########
CHANGELOG.md:
##########
@@ -34,6 +34,7 @@ and this project adheres to [Semantic
Versioning](https://semver.org/).
* AGS1 readers now require `StandardKeyMetadata::file_length` and reject
missing or invalid lengths without falling back to a storage stat.
AGS1-encrypted manifests, manifest lists, and Puffin files written by earlier
development builds without this field must be rewritten using a build that can
still read them before upgrading. This matches the Java client's read contract.
* `iceberg_catalog_loader::BoxedCatalogBuilder` has a new required method,
`with_runtime`. Types implementing `CatalogBuilder` get it through the blanket
impl; types implementing `BoxedCatalogBuilder` directly (for example, a wrapper
around another `Box<dyn BoxedCatalogBuilder>`) must implement it, typically by
forwarding the runtime to the builder they wrap.
* `Transaction::commit` now returns `ErrorKind::PreconditionFailed` for
read-only tables (`Table::readonly()`, e.g. from `StaticTable::into_table` or
`TableBuilder::readonly(true)`), including empty transactions, which previously
returned the table unchanged. Java treats an empty transaction on a static
table as a no-op.
+* `ArrowReader` reads nested struct fields as null for Parquet files that have
neither field ids nor a name mapping, as iceberg-java does. Earlier versions
returned their stored values by position. Tables migrated without field ids
need a name mapping (`schema.name-mapping.default`) that covers nested fields
to read them.
Review Comment:
Follow-up: "read as null" only holds for optional fields. Required nested
fields now fail with `DataInvalid`, and nested fields with an `initial-default`
hit the #3261 `FeatureUnsupported` error, and both used to read by position.
Since this is in the breaking-changes list, I'd name both cases at some point
so someone upgrading a migrated table isn't surprised by a hard failure.
##########
crates/iceberg/src/arrow/record_batch_transformer.rs:
##########
@@ -270,19 +270,39 @@ impl PromotePlan {
}),
(DataType::Map(source_entries, _), DataType::Map(target_entries,
sorted)) => {
match (source_entries.data_type(), target_entries.data_type())
{
- (DataType::Struct(source_fields),
DataType::Struct(target_fields)) => {
+ // Key and value match by position, as in iceberg-java's
map readers.
+ (DataType::Struct(source_fields),
DataType::Struct(target_fields))
+ if source_fields.len() == target_fields.len() =>
+ {
Ok(PromotePlan::Map {
- entries: Self::build_struct_children(
- source_fields,
- target_fields,
- snapshot_schema,
- constant_fields,
- )?,
+ entries: source_fields
+ .iter()
+ .zip(target_fields.iter())
+ .enumerate()
+ .map(|(source_index, (source_field,
target_field))| {
+ Ok(ChildPlan::FromSource {
+ source_index,
+ plan: Self::build(
+ source_field.data_type(),
+ target_field.data_type(),
+ snapshot_schema,
+ constant_fields,
+ )?,
+ })
+ })
+ .collect::<Result<_>>()?,
entry_fields: target_fields.clone(),
field: target_entries.clone(),
sorted: *sorted,
})
}
+ (DataType::Struct(source_fields),
DataType::Struct(target_fields)) => {
+ Err(invalid_data!(
+ "map entries have {} source fields, expected {}",
Review Comment:
nit, follow-up: this renders as "map entries have 1 source fields, expected
2". Something like `"expected {} map entry fields, got {}"` (arguments swapped)
avoids the plural problem. The test string at L1767 needs the same change.
--
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]
