Repository navigation
fix: fill schema-added nested columns with typed NULL arrays on read - #2635
Conversation
When a column of a nested type (list, map, or a struct with nested children) is added to the table schema after data files were written, reading those older files failed with "unexpected target column type": the helpers that materialize missing columns only handled primitive types plus structs with primitive-only children, via hand-written per-type NULL branches. Build the all-NULL column with arrow's new_null_array instead, which supports every Arrow type (including arbitrarily nested ones), and drop the per-type NULL branches from create_primitive_array_repeated and create_primitive_array_single_element. Closes apache#2618
| // With no value, the single element is NULL. `new_null_array` supports every | ||
| // Arrow type, including nested ones (list/map/struct), which matters for | ||
| // columns added by schema evolution after a data file was written (#2618). | ||
| if prim_lit.is_none() { |
There was a problem hiding this comment.
Hi @viirya thanks for the fix. We encountered the same issue with a schema evolution case: a list of binary was added, and the original code cannot handle it. We fixed that internally and are about to contribute it back.
Our fix is similar with yours. However during the internal code review process, we noticed the function name create_primitive_array_single_element is no longer valid, it's not just creating primitive array any more, it also creating list/struct/map arrays now. Considering the default value in Iceberg V3, a struct could have default values. I think it would be best to change the signature to create_array_single_element snd passes prim_lit as &Option<Literal>, WDYT?
There was a problem hiding this comment.
I think this fix is valid short-term fix. We can get it merged first, and refactor it in a follow-up PR to address the naming issue and default value support in Iceberg V3.
There was a problem hiding this comment.
Thanks @advancedxy — agreed on both counts, and thanks for flagging the V3 angle.
You're right that create_primitive_* is now a misnomer since these functions materialize list/map/struct NULLs too. I dug into the V3 default-value direction a bit, and it's actually a slightly larger refactor than just the helper signatures: the value currently threaded into them is Option<PrimitiveLiteral> (via ColumnSource::Add), and generate_transform_operations deliberately drops any non-primitive initial_default today (if let Literal::Primitive(prim) = lit { .. } else { None } in record_batch_transformer.rs). So supporting a struct/nested default would mean widening ColumnSource::Add.value to Option<Literal> and the transformer's default-extraction alongside the rename to create_array_single_element.
Given that, I'd prefer to keep this PR as the focused wraparound/nested-NULL fix and do the rename + Literal widening + V3 default support together in the follow-up, so the type change lands in one coherent step rather than renaming now and re-touching the signature later. Happy to open the follow-up issue/PR for that.
There was a problem hiding this comment.
Given that, I'd prefer to keep this PR as the focused wraparound/nested-NULL fix and do the rename + Literal widening + V3 default support together in the follow-up
Agreed. Let's merge this first.
There was a problem hiding this comment.
Given that, I'd prefer to keep this PR as the focused wraparound/nested-NULL fix and do the rename +
Literalwidening + V3 default support together in the follow-up, so the type change lands in one coherent step rather than renaming now and re-touching the signature later. Happy to open the follow-up issue/PR for that.
Has that follow-up issue been filed? I couldn't find one. Could you open it and reference it in the new TODO so the rename and the struct default work stay connected to this PR? #3261 tracks applying initial-default to a nested field missing from an existing struct. Materializing a {} struct default needs the same per-child default logic, so extending #3261 to cover both would also work.
|
cc @kevinjqliu |
kevinjqliu
left a comment
kevinjqliu
left a comment
There was a problem hiding this comment.
Thanks! A couple questions from me. Would be good to have others take a look too
BTW I also notice there's another PR addressing the same issue (#2649)
| } | ||
|
|
||
| #[test] | ||
| fn schema_evolution_adds_list_map_and_nested_struct_columns_with_nulls() { |
There was a problem hiding this comment.
nit: this test is a bit difficult to read. do you mind ordering so its easier to decipher the test?
we're using an evolved schema (with new optional list/map/struct types) to read the a data file that has the older schema. And we expect that all new optional list/map/struct types to be null.
should we also test for the shape of the null value? e.g. the null struct still has the expected child fields?
| // With no value, the single element is NULL. `new_null_array` supports every | ||
| // Arrow type, including nested ones (list/map/struct), which matters for | ||
| // columns added by schema evolution after a data file was written (#2618). |
There was a problem hiding this comment.
nit: feels like we're over explaining here, wdyt?
There was a problem hiding this comment.
maybe we should document the 1, i think its for new_null_array 's num_rows
| Ok(Arc::new(array)) | ||
| } | ||
| } | ||
| (DataType::Timestamp(TimeUnit::Microsecond, timezone), None) => { |
There was a problem hiding this comment.
just double checking, do you know if new_null_array handles this and the nanosecond case correctly?
There was a problem hiding this comment.
Yes. new_null_array builds from the DataType, and for timestamps the timezone is part of DataType::Timestamp(unit, tz) (and precision/scale are part of Decimal128), so the null array carries the exact same type the old per-type branches produced, including Timestamp(Nanosecond, tz). The thing it does not preserve is RunEndEncoded wrapping, but the NULL-fill path uses the plain schema type (REE is only applied on the Some/constant path via datum_to_arrow_type_with_ree), so there is no regression. A test asserting the full nested DataType (which kevinjqliu also asked for) would lock this in.
| })?; | ||
| Ok(Arc::new(array)) | ||
| } | ||
| (DataType::Struct(fields), None) => { |
There was a problem hiding this comment.
nice, glad we can get rid of these with the helper method!
| /// | ||
| /// This is used for creating non-constant arrays where we need the same value | ||
| /// repeated for each row. | ||
| pub(crate) fn create_primitive_array_repeated( |
There was a problem hiding this comment.
maybe a good opportunity for refactoring with create_primitive_array_repeated, i see a lot of repetition. but its outside this pr's scope 😄
mbutrovich
left a comment
mbutrovich
left a comment
There was a problem hiding this comment.
Building on kevinjqliu's note that #2649 addresses the same issue: this is the one to take. #2649 is an additive per-type fix (+80/-1); this PR's new_null_array approach is more general, removes the existing per-type branches rather than extending them, and its test covers a struct whose child is itself a list (s: struct<a, ys: list<long>>) that an enumerate-each-type fix would miss. Recommend landing this and closing #2649 as redundant (the bug still gets fixed and #2649's intent is fully covered here).
Thanks @viirya!
| Ok(Arc::new(array)) | ||
| } | ||
| } | ||
| (DataType::Timestamp(TimeUnit::Microsecond, timezone), None) => { |
There was a problem hiding this comment.
Yes. new_null_array builds from the DataType, and for timestamps the timezone is part of DataType::Timestamp(unit, tz) (and precision/scale are part of Decimal128), so the null array carries the exact same type the old per-type branches produced, including Timestamp(Nanosecond, tz). The thing it does not preserve is RunEndEncoded wrapping, but the NULL-fill path uses the plain schema type (REE is only applied on the Some/constant path via datum_to_arrow_type_with_ree), so there is no regression. A test asserting the full nested DataType (which kevinjqliu also asked for) would lock this in.
Address review feedback on apache#2635: - Extract the evolved schema into `schema_with_added_nested_columns` and reorder the test to read top-to-bottom as a story (old file -> evolved schema -> expect typed NULLs), per @kevinjqliu. - Assert the all-NULL struct still carries its full nested shape (child fields `a` and the nested list `ys`), locking in that the type-preserving new_null_array fill keeps nested children. - Trim the over-explanatory comments on the new_null_array NULL-fill paths and note that the literal `1` is new_null_array's row count.
|
Thanks @kevinjqliu — addressed the feedback in e845ae0:
|
|
Thanks @mbutrovich for reviewing. |
|
@kevinjqliu Could you take another look? Thanks! |
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions. |
|
@kevinjqliu @blackmwk or @CTTY can you take a look? Thanks! |
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions. |
laskoviymishka
left a comment
laskoviymishka
left a comment
There was a problem hiding this comment.
agreed with @mbutrovich, this is the one to take. Collapsing the per-type None branches into new_null_array is the right generalization, and the struct-with-a-nested-list test (s: struct<a, ys: list<long>>) pins exactly the case a per-type fix would miss. Happy to see #2649 close as redundant.
One thing worth flagging, and it's explicitly not a blocker: now that the null path works for nested types, a newly-added column whose initial_default is a non-primitive Literal (struct/list/map) gets silently dropped to None by the and_then in record_batch_transformer.rs (~L872) and null-filled — where the spec says a struct added with initial_default = {} should read back as a non-null struct with each child's own default applied. It's a pre-existing gap, just more visible now, and the fix is out of scope here. I'd file a follow-up (materialize complex defaults, or reject them in NestedField::with_initial_default to match Java's castDefault) and drop a // TODO at that and_then so it doesn't get lost.
Left a couple of small inline nits too: the retained (DataType::Null, _) arm reads as if it still handles None, and it'd be nice to pin the child field-ids in the new test.
None of that blocks; this is good to merge, will left soaking for some time before merging.
| Some(NullBuffer::new_null(num_rows)), | ||
| )) | ||
| } | ||
| (DataType::Null, _) => Arc::new(arrow_array::NullArray::new(num_rows)), |
There was a problem hiding this comment.
Now that the early return catches every prim_lit.is_none() case up top, this arm is only reachable with Some(_) — a bare Null type with no literal already returns above. Harmless, but it reads as if it still handles the None case. I'd tighten it to (DataType::Null, Some(_)) or just drop it. wdyt?
There was a problem hiding this comment.
Good point — changed this to (DataType::Null, Some(_)). This makes the reachable case explicit while preserving the existing behavior for Null with a supplied literal; dropping the arm would make that case return an error.
| NestedField::optional( | ||
| 7, | ||
| "s", | ||
| Type::Struct(crate::spec::StructType::new(vec![ |
There was a problem hiding this comment.
s is added here with no initial_default, so null-fill is exactly right. The case this PR quietly makes reachable is a struct added with a non-primitive initial_default (e.g. {}) — the and_then in the transformer drops it to None, so we'd null-fill instead of materializing the spec-mandated non-null struct. Not blocking, but a small pin-test here (even #[ignore]'d) documenting that gap would pair well with the follow-up I noted in the summary.
There was a problem hiding this comment.
Agreed — the extraction currently drops non-primitive defaults to None. Added a TODO at that and_then to keep the gap visible. I'd keep the regression test with the complex-default implementation in a follow-up, where we can cover the non-null struct and child-default semantics together.
| panic!("field `s` should be a struct"); | ||
| }; | ||
| let child_names: Vec<&str> = struct_fields.iter().map(|f| f.name().as_str()).collect(); | ||
| assert_eq!(child_names, vec!["a", "ys"]); |
There was a problem hiding this comment.
Nice — pinning that the null struct keeps a and ys (and that ys stays a List) is exactly the shape a per-type fix would flatten. Could we also assert the child fields keep their PARQUET_FIELD_IDs (8/9 here, plus 3/5/6 on the top-level list/map)? That metadata is the load-bearing guarantee of this approach and what downstream name-matching relies on, so it'd be good to guard against a refactor that strips it. wdyt?
There was a problem hiding this comment.
Added assertions for PARQUET_FIELD_ID on a/ys (8/9), the top-level list element (3), and the map key/value (5/6). I also included the nested ys element (10), so the test covers metadata preservation through that extra nesting level.
|
Thanks @laskoviymishka for the approval and the detailed feedback! Updated in cc794c8, which also merges the latest upstream main and resolves the conflicts. I tightened the All 1,675 iceberg library tests pass locally, along with clippy and formatting checks. |
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Fixes schema evolution reads when newly-added nested columns (list/map/nested struct children) are missing in older files by generating correctly-typed all-NULL Arrow arrays for any DataType.
Changes:
- Replace per-type
Nonehandling witharrow_array::new_null_arrayfor both single-element and repeated NULL materialization. - Add regression test covering schema evolution that adds list, map, and a struct containing a nested list.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| crates/iceberg/src/arrow/value.rs | Use new_null_array to create typed NULL arrays for any Arrow type, removing many hand-written NULL branches. |
| crates/iceberg/src/arrow/record_batch_transformer.rs | Add regression test ensuring evolved nested columns are filled with typed all-NULL arrays and preserve nested field IDs. |
Suppressed comments (1)
crates/iceberg/src/arrow/value.rs:713
create_primitive_array_single_elementno longer has an explicitDataType::Nullarm. If this function is ever called withdata_type == DataType::Nullandprim_lit.is_some(), it will now error via the catch-all, whilecreate_primitive_array_repeatedexplicitly accepts(DataType::Null, Some(_)). Consider adding a(DataType::Null, Some(_)) => Ok(Arc::new(arrow_array::NullArray::new(1)))arm (or equivalent) to keep behavior consistent and avoid unexpected failures.
})?;
Ok(Arc::new(array))
}
_ => Err(Error::new(
ErrorKind::Unexpected,
format!("Unsupported constant type combination: {data_type:?} with {prim_lit:?}"),
)),
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
@viirya i think this is good to merge, fix the conflicts and we are good. |
There was a problem hiding this comment.
I hope to follow-up on this soon so not a big deal, but in general I'd prefer we fail closed if we cannot support this.
I was planning to introduce this:
let default_value = match iceberg_field.initial_default.as_ref() {
None => None,
Some(Literal::Primitive(prim)) => Some(prim.clone()),
Some(_) => {
return Err(Error::new(
ErrorKind::FeatureUnsupported,
format!(
"Cannot read field {} that is absent from the data file: \
applying a non-primitive initial-default for {} is not yet \
supported",
iceberg_field.name, iceberg_field.field_type
),
));
}
};i.e. this PR will take an obscure failure reading data files without the complex column, into implementing null filling for complex columns without a default and properly reporting a clear error for those with a default.
There was a problem hiding this comment.
Agreed — silently replacing an unsupported default with NULL could return incorrect data. Applied your suggested match in fbc8f72: absent optional fields without a default still get typed NULLs, primitive defaults retain their existing behavior, and non-primitive defaults return FeatureUnsupported with the field name and type.
Added regression coverage for both optional and required structs with an initial default. The TODO now tracks implementing complex defaults, including child defaults.
There was a problem hiding this comment.
Thanks for addressing this one!
Head branch was pushed to by a user without write access
|
Thanks! Merged the latest upstream main and resolved the conflicts in fbc8f72. I also addressed @dannycjones’s suggestion: missing fields with non-primitive initial defaults now return |
laskoviymishka
left a comment
laskoviymishka
left a comment
There was a problem hiding this comment.
The error-path rework all landed — the and_then is now an explicit match that returns FeatureUnsupported instead of silently null-filling, the // TODO: Materialize complex initial defaults is in, the new schema_evolution_absent_struct_with_initial_default_errors test covers both required and optional, and the regression test pins the child field-ids (3/5/6/8/9/10). That's exactly the direction I was hoping for — erroring beats handing back a wrong value for an absent nested-default field.
One framing thing worth settling before merge: the FeatureUnsupported kind + "not yet supported" wording + the TODO read as if materializing a non-primitive initial-default is a feature we'll grow into — but Java's castDefault rejects a non-null default on a nested-type field outright, so it's invalid metadata, not a missing feature. I'd either reword so we're not promising nested-default materialization, or switch to DataInvalid to line up with castDefault. I lean toward the latter, but either's fine if it's deliberate — wdyt?
Agreed with @dannycjones that this flips a previously-successful read (a file predating a struct/list/map initial_default) into a hard error. That's the right trade since the old read was silently wrong, but it's worth a line in the PR description/changelog so it isn't a surprise.
Left a few small nits inline — a direct unit test for the collapsed None paths, a comment trim, and a required= label on the error test. None block. This is good; happy with it. Will keep soak for some time before merge.
| .unwrap(); | ||
|
|
||
| let err = transformer.process_record_batch(file_batch).unwrap_err(); | ||
| assert_eq!(err.kind(), crate::ErrorKind::FeatureUnsupported); |
There was a problem hiding this comment.
The loop runs both required values but the asserts don't say which case failed. I'd thread required into the messages ("required={required}") or split into two #[test]s, so a failure points at the right branch.
There was a problem hiding this comment.
Added required={required} to the assertions and the unexpected-success message, so failures identify which case was running.
| data_type: &DataType, | ||
| prim_lit: Option<&PrimitiveLiteral>, | ||
| ) -> Result<ArrayRef> { | ||
| // No value: a single NULL of any (possibly nested) type (#2618). The `1` is |
There was a problem hiding this comment.
Small thing while we're here: the inline #2618 goes stale once this merges, and spelling out that 1 is the row count narrates the call. I'd trim to just the intent — "single NULL of any (possibly nested) type."
There was a problem hiding this comment.
Trimmed the comment to the intent and removed the issue reference and row-count explanation. Also removed the issue reference from the repeated helper’s comment.
| // No value: a single NULL of any (possibly nested) type (#2618). The `1` is | ||
| // `new_null_array`'s row count. | ||
| if prim_lit.is_none() { | ||
| return Ok(new_null_array(data_type, 1)); |
There was a problem hiding this comment.
This collapses the Decimal / Timestamp-with-tz / Struct / Null None arms into one new_null_array call, but nothing exercises either helper with None directly anymore — the transformer tests only reach it through the full read path. I'd add a small table-driven test over both create_primitive_array_single_element and _repeated for Decimal128(10,2), Timestamp(µs, Some("+00:00")), a nested Struct, and Null, asserting data type, length, and null_count. That re-pins the timezone/precision preservation the deleted arms carried, and covers num_rows == 0.
There was a problem hiding this comment.
Added a table-driven test covering both helpers with None for Decimal128(10, 2), a microsecond timestamp with +00:00, a struct containing a list, and Null. It checks the exact data type, length, and NULL count, including repeated arrays with 0, 1, and 3 rows.
One Arrow detail: NullArray has no validity bitmap, so its physical null_count() is zero. The test checks logical_null_count() for every type and additionally checks physical null_count() for the other types.
|
Thanks for the follow-up! I checked the spec against Java’s I’m keeping The PR description now calls out the behavior change from potentially returning incorrect NULLs to reporting an explicit error. I also addressed the three inline suggestions. |
mbutrovich
left a comment
mbutrovich
left a comment
There was a problem hiding this comment.
Thanks for the updates, @viirya. I agree with keeping FeatureUnsupported. The spec's default values section allows a nested struct's initial-default to be a non-null struct with no field values ({}), so a struct default is a case this reader doesn't implement yet, not invalid metadata.
I also wrote an old Parquet file with only id and read it through ArrowReader against a schema that adds a list, a map, and a struct containing a list. On the head commit the read succeeds, each added column is all NULL, and the batch schema equals schema_to_arrow_schema of the table schema.
| // (3b) The added columns carry the evolved schema's Arrow types and are all-NULL. | ||
| assert!(matches!( | ||
| result.schema().field(1).data_type(), | ||
| DataType::List(_) | ||
| )); | ||
| assert!(matches!( | ||
| result.schema().field(2).data_type(), | ||
| DataType::Map(_, _) | ||
| )); | ||
| for (idx, name) in [(1, "xs"), (2, "props"), (3, "s")] { | ||
| assert_eq!( | ||
| result.column(idx).null_count(), | ||
| 3, | ||
| "added nested column `{name}` should be all-NULL" | ||
| ); | ||
| } | ||
|
|
||
| // (3c) The all-NULL struct still carries its full nested shape (`a` plus the | ||
| // nested list `ys`), not a degenerate empty struct — this is what the | ||
| // type-preserving NULL fill guarantees over an enumerate-each-type fix. | ||
| let result_schema = result.schema(); | ||
| let DataType::Struct(struct_fields) = result_schema.field(3).data_type() else { | ||
| panic!("field `s` should be a struct"); | ||
| }; | ||
| let child_names: Vec<&str> = struct_fields.iter().map(|f| f.name().as_str()).collect(); | ||
| assert_eq!(child_names, vec!["a", "ys"]); | ||
|
|
||
| let DataType::List(xs_element) = result_schema.field(1).data_type() else { | ||
| panic!("field `xs` should be a list"); | ||
| }; | ||
| let DataType::Map(entries, _) = result_schema.field(2).data_type() else { | ||
| panic!("field `props` should be a map"); | ||
| }; | ||
| let DataType::Struct(map_fields) = entries.data_type() else { | ||
| panic!("map entries should be a struct"); | ||
| }; | ||
| let DataType::List(ys_element) = struct_fields[1].data_type() else { | ||
| panic!("field `ys` should be a list"); | ||
| }; | ||
| for (field, expected_id) in [ | ||
| (xs_element, "3"), | ||
| (&map_fields[0], "5"), | ||
| (&map_fields[1], "6"), | ||
| (&struct_fields[0], "8"), | ||
| (&struct_fields[1], "9"), | ||
| (ys_element, "10"), | ||
| ] { | ||
| assert_eq!( | ||
| field | ||
| .metadata() | ||
| .get(PARQUET_FIELD_ID_META_KEY) | ||
| .map(String::as_str), | ||
| Some(expected_id), | ||
| "nested field `{}` should preserve its field ID", | ||
| field.name() | ||
| ); | ||
| } |
There was a problem hiding this comment.
What do you think about asserting the whole result schema here instead of checking types, child names, and field IDs one at a time? The transformer builds its target types with schema_to_arrow_schema, and Arrow's Field equality compares names, nullability, data types, and metadata at every nesting level. One assert_eq! covers everything these checks cover (including the field IDs 3/5/6/8/9/10) and also catches nullability changes. I tried it on the head commit: the test passes, and dropping the PARQUET:field_id from the ys element in the expected schema makes the comparison fail.
This needs use super::{field_with_id, schema_to_arrow_schema}; in place of the PARQUET_FIELD_ID_META_KEY import at line 1156, which is otherwise unused after the change.
| // (3b) The added columns carry the evolved schema's Arrow types and are all-NULL. | |
| assert!(matches!( | |
| result.schema().field(1).data_type(), | |
| DataType::List(_) | |
| )); | |
| assert!(matches!( | |
| result.schema().field(2).data_type(), | |
| DataType::Map(_, _) | |
| )); | |
| for (idx, name) in [(1, "xs"), (2, "props"), (3, "s")] { | |
| assert_eq!( | |
| result.column(idx).null_count(), | |
| 3, | |
| "added nested column `{name}` should be all-NULL" | |
| ); | |
| } | |
| // (3c) The all-NULL struct still carries its full nested shape (`a` plus the | |
| // nested list `ys`), not a degenerate empty struct — this is what the | |
| // type-preserving NULL fill guarantees over an enumerate-each-type fix. | |
| let result_schema = result.schema(); | |
| let DataType::Struct(struct_fields) = result_schema.field(3).data_type() else { | |
| panic!("field `s` should be a struct"); | |
| }; | |
| let child_names: Vec<&str> = struct_fields.iter().map(|f| f.name().as_str()).collect(); | |
| assert_eq!(child_names, vec!["a", "ys"]); | |
| let DataType::List(xs_element) = result_schema.field(1).data_type() else { | |
| panic!("field `xs` should be a list"); | |
| }; | |
| let DataType::Map(entries, _) = result_schema.field(2).data_type() else { | |
| panic!("field `props` should be a map"); | |
| }; | |
| let DataType::Struct(map_fields) = entries.data_type() else { | |
| panic!("map entries should be a struct"); | |
| }; | |
| let DataType::List(ys_element) = struct_fields[1].data_type() else { | |
| panic!("field `ys` should be a list"); | |
| }; | |
| for (field, expected_id) in [ | |
| (xs_element, "3"), | |
| (&map_fields[0], "5"), | |
| (&map_fields[1], "6"), | |
| (&struct_fields[0], "8"), | |
| (&struct_fields[1], "9"), | |
| (ys_element, "10"), | |
| ] { | |
| assert_eq!( | |
| field | |
| .metadata() | |
| .get(PARQUET_FIELD_ID_META_KEY) | |
| .map(String::as_str), | |
| Some(expected_id), | |
| "nested field `{}` should preserve its field ID", | |
| field.name() | |
| ); | |
| } | |
| // (3b) The added columns carry the evolved schema's Arrow types, including nested | |
| // field names and field IDs, and are all-NULL. | |
| let expected_schema = schema_to_arrow_schema(&schema_with_added_nested_columns()).unwrap(); | |
| assert_eq!(result.schema().as_ref(), &expected_schema); | |
| for (idx, name) in [(1, "xs"), (2, "props"), (3, "s")] { | |
| assert_eq!( | |
| result.column(idx).null_count(), | |
| 3, | |
| "added nested column `{name}` should be all-NULL" | |
| ); | |
| } |
There was a problem hiding this comment.
Agreed — replaced the individual type, name, and field-ID checks with a full schema comparison using schema_to_arrow_schema, while keeping the per-column NULL-count assertions. This also covers nested nullability. Updated the import as suggested; the transformer tests pass.
|
Thanks for checking the spec and testing the full I’ll use #3261 for the follow-up and add the missing-parent |
mbutrovich
left a comment
mbutrovich
left a comment
There was a problem hiding this comment.
LGTM, thanks @viirya!
|
Thanks all for reviewing this PR! |
The only conflict was the transformer test imports. Keep schema_to_arrow_schema from apache#2635 and drop the paths that became redundant.
Which issue does this PR close?
What changes are included in this PR?
Reading older data files after optional nested columns are added to the table schema can fail when materializing missing columns as NULL arrays. This PR uses Arrow's
new_null_arrayin bothcreate_primitive_array_single_elementandcreate_primitive_array_repeated, replacing the per-type NULL branches with support for nested types while preserving the requested Arrow data type, including decimal precision/scale, timestamp timezone, and nested field metadata.Behavior change for initial defaults: When a field is absent from a data file and has a non-primitive
initial-default, reading now returns a descriptiveFeatureUnsupportederror instead of dropping the default and potentially returning an incorrect NULL value. Missing optional fields without a default still receive typed NULLs, and primitive defaults retain their existing behavior.This error kind is intentional: the Iceberg specification permits a non-null struct default
{}, whose values are derived from each child's defaults. Materializing that default is not implemented here; rejecting it avoids silently returning incorrect data without classifying all non-primitive defaults as invalid metadata. This does not imply that arbitrary complex defaults are valid.Are these changes tested?
schema_evolution_adds_list_map_and_nested_struct_columns_with_nullsreads a batch containing onlyidagainst an evolved schema with a list, a map, and a struct containing a nested list. It verifies typed NULLs, nested shape, and child field IDs 3/5/6/8/9/10.schema_evolution_absent_struct_with_initial_default_errorsverifies that absent structs with an initial default returnFeatureUnsupported, for both optional and required fields.