feat(spec): add unknown primitive type support - #2773
Conversation
0a16bfa to
0144610
Compare
7e87c07 to
e379217
Compare
There was a problem hiding this comment.
Pull request overview
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.
Adds PrimitiveType::Unknown support across the Iceberg Rust type system, including conversions and validation, while ensuring Parquet/Avro/Catalog integrations behave predictably when unknown types appear.
Changes:
- Introduces
PrimitiveType::Unknownand wires it through Arrow/Avro conversions, Parquet read/write behavior, and schema validation. - Ensures unknown fields are logically null (e.g., omitted from Parquet physical writes, ignored during Parquet reads/predicate projection).
- Updates catalog schema conversion (Hive/Glue) to explicitly reject unknown types.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/iceberg/src/writer/file_writer/parquet_writer.rs | Projects incoming batches to a Parquet-write schema that omits unknown fields; adds tests for projection behavior. |
| crates/iceberg/src/spec/values/datum.rs | Rejects constructing a Datum from bytes for Unknown. |
| crates/iceberg/src/spec/schema/mod.rs | Adds schema-level validation enforcing unknown fields are optional and defaults are null-only; adds related tests. |
| crates/iceberg/src/spec/datatypes.rs | Adds Unknown primitive; updates serde handling for defaults to validate unknown defaults are null-only; updates format version logic and tests. |
| crates/iceberg/src/avro/schema.rs | Maps unknown to Avro null, avoids redundant optional wrapping for null, and round-trips unknown through Avro. |
| crates/iceberg/src/arrow/value.rs | Treats unknown as all-null during Arrow->Iceberg literal conversion; tightens Null array creation semantics + tests. |
| crates/iceberg/src/arrow/schema.rs | Maps Arrow Null <-> Iceberg Unknown; adds Parquet-write Arrow schema builder that omits unknown physical fields + tests. |
| crates/iceberg/src/arrow/record_batch_transformer.rs | Rebuilds arrays when schemas include Null children (unknowns omitted from Parquet structs), ensuring missing nested children are filled with nulls; adds tests. |
| crates/iceberg/src/arrow/reader/projection.rs | Ensures unknown fields are never read/projected from Parquet; adjusts fallback ID assignment to account for omitted unknown fields; adds tests. |
| crates/iceberg/src/arrow/reader/pipeline.rs | Passes Iceberg schema into fallback field-id assignment / projection mapping. |
| crates/iceberg/src/arrow/nan_val_cnt_visitor.rs | Skips unknown fields during NaN counting and adds explicit traversal logic. |
| crates/iceberg/public-api.txt | Exposes PrimitiveType::Unknown in public API surface. |
| crates/catalog/hms/src/schema.rs | Rejects unknown type when converting to Hive schema. |
| crates/catalog/glue/src/schema.rs | Rejects unknown type when converting to Glue schema. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
60872f6 to
5941ec2
Compare
blackmwk
left a comment
There was a problem hiding this comment.
Found two data-correctness issues that need to be addressed before this can merge.
Missing top-level container defaults
RecordBatchTransformer retains an initial_default only when it is Literal::Primitive. If a top-level struct, list, or map is absent from an older file, the reader creates nulls (or violates a required field) rather than materializing the declared default. Please retain the full literal and use create_literal_array_repeated on this path, with a regression test for a missing top-level container default.
The inline comment covers the fallback-ID issue.
This review was drafted by an AI-assisted tool and confirmed by an Apache Iceberg Rust maintainer. After you've addressed the points above and pushed an update, an Apache Iceberg Rust maintainer — a real person — will take the next look at the PR. The findings cite the project's review criteria; if you think one of them is mis-applied, please reply on the PR and a maintainer will weigh in.
More on how Apache Iceberg Rust handles maintainer review:
CONTRIBUTING.md.
|
Also this pr is too large to review, I would suggest to split them using gh stack to smaller prs. |
c4236cd to
11c8ef1
Compare
11c8ef1 to
02756bf
Compare
Co-authored-by: Codex <codex@openai.com>
02756bf to
a7efd92
Compare
Which issue does this PR close?
No issue is currently linked.
What changes are included in this PR?
This PR is the first independently mergeable slice of the Unknown-type work:
PrimitiveType::Unknownparsing, serialization, schema/default validation, format-version checks, and public API support.Nulland Avronullto the Unknown type and preserves null-only values.The original change is now split into these upstream PRs:
This PR deliberately excludes the Parquet read/write behavior. The follow-up drafts are currently cumulative and will be rebased after their dependencies merge.
Two unrelated fixes formerly carried by this branch were extracted into independent upstream drafts: #3070 and #3071.
Are these changes tested?
cargo test -p iceberg --lib(1,588 passed)make check-fmtmake check-clippygit diff --checkAI Disclosure
OpenAI Codex was used for implementation, regression-test scaffolding, review-feedback follow-up, and splitting the original change into independently reviewable commits. The resulting changes were reviewed against repository conventions and validated with the test and lint coverage listed above.