comphead commented on code in PR #3437:
URL: https://github.com/apache/iceberg-rust/pull/3437#discussion_r4287482408
##########
crates/iceberg/src/arrow/record_batch_transformer.rs:
##########
@@ -270,14 +270,27 @@ 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() =>
Review Comment:
When the field counts differ, this guard falls through to the `_` arm, which
reports "expected struct-typed map entries" even though the entries are
structs. A separate message such as "map entries have N source fields, expected
M" would point at the real problem.
##########
crates/iceberg/src/arrow/record_batch_transformer.rs:
##########
@@ -1855,7 +1860,7 @@ mod test {
}
#[test]
- fn promote_idless_nested_struct_keeps_data() {
+ fn promote_idless_nested_struct_reads_null() {
Review Comment:
This test looks redundant. In a mutation spot check, every production
mutation it caught was also caught by another test:
- Restoring the positional fallback in `build_struct_children` failed this
test, `promote_idless_map_struct_value_reads_null_fields`, and the
`ArrowReader` test.
- Making absent fields non-null failed this test along with most of the
other promote tests.
- Passing `None` for struct validity in `apply_struct` did not fail it.
The outer struct is fully absent, so the inner struct is never visited and
the test adds no path beyond
`promote_struct_without_source_field_ids_reads_null`. Consider dropping it.
Likewise `promote_idless_map_keeps_data` is now caught together with
`promote_idless_map_struct_value_reads_null_fields` for the map change
(mutation: build map entries by id again), so one map test with a struct value
would be enough. This is a spot check on four mutations, not a coverage proof.
--
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]
