feat(spec): add unknown datatype support - #1
Conversation
💡 Codex Reviewiceberg-rust/crates/iceberg/src/spec/schema/mod.rs Lines 199 to 200 in d56cd19  Reject non-null defaults during unknown-field deserialization
When table metadata JSON contains an ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex fix test failure |
|
Summary
Testing
Committed the changes with commit |
|
@codex check review comments and fix test failure |
|
Summary
Committed the changes as Testing
|
013362e to
043dbd5
Compare
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 043dbd5bb4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 043dbd5bb4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
3e9e57e to
89e1ac1
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds end-to-end support for Iceberg’s unknown primitive type across the Rust type system and its integration layers (Arrow, Avro, catalogs), while also tightening format-version validation (notably for v3-only nanosecond timestamps) and stabilizing predicate display ordering for deterministic tests.
Changes:
- Introduces
PrimitiveType::Unknownplus schema-level validation rules (must be optional; defaults must be null/absent) and propagates it through Arrow/Avro conversion paths as null-only. - Adds schema/type compatibility validation against table format version during metadata normalization and schema additions; derives a minimum required format version from schema types to set table creation format version when needed.
- Makes set-predicate stringification deterministic by sorting literal lists (updating many expected strings in transform/projection tests).
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/integrations/datafusion/src/schema.rs | Uses schema-derived minimum format version (e.g., timestamp_ns ⇒ v3) when registering tables; adds a regression test for v3 selection. |
| crates/iceberg/src/transform/truncate.rs | Updates expected IN (...) ordering in tests to match deterministic literal ordering. |
| crates/iceberg/src/transform/temporal.rs | Updates expected IN (...) ordering in tests to match deterministic literal ordering across temporal transforms. |
| crates/iceberg/src/transform/bucket.rs | Updates expected IN (...) ordering in tests to match deterministic literal ordering for bucket transforms. |
| crates/iceberg/src/spec/values/datum.rs | Rejects constructing datums from bytes for unknown (feature unsupported). |
| crates/iceberg/src/spec/table_metadata.rs | Validates all schemas are compatible with table format version during normalization; adds tests for v3-only types rejected in v2 metadata/builder flows. |
| crates/iceberg/src/spec/table_metadata_builder.rs | Validates schemas added via builder are compatible with the table format version. |
| crates/iceberg/src/spec/schema/mod.rs | Enforces unknown-field constraints at schema build; adds min-format-version calculation and format-version compatibility validation; adds tests. |
| crates/iceberg/src/spec/datatypes.rs | Adds PrimitiveType::Unknown, format-version requirement helpers, and makes NestedField deserialization fallible to enforce default validation. |
| crates/iceberg/src/expr/visitors/strict_projection.rs | Updates expected predicate strings due to deterministic literal ordering. |
| crates/iceberg/src/expr/predicate.rs | Sorts set-expression literals for deterministic Display; updates tests accordingly. |
| crates/iceberg/src/avro/schema.rs | Maps unknown to Avro null and adjusts optional/union handling to avoid null-of-null; roundtrip tests added. |
| crates/iceberg/src/arrow/value.rs | Converts unknown values to null literals; adds Arrow Null handling for constant array creation. |
| crates/iceberg/src/arrow/schema.rs | Maps Arrow DataType::Null ⇔ Iceberg PrimitiveType::Unknown; adds roundtrip tests. |
| crates/iceberg/src/arrow/reader/projection.rs | Treats unknown as non-leaf (no parquet column read) and compatible with any file primitive type. |
| crates/iceberg/public-api.txt | Records Schema::min_format_version in the public API surface. |
| crates/catalog/hms/src/schema.rs | Rejects Hive schema conversion for unknown (feature unsupported). |
| crates/catalog/glue/src/schema.rs | Rejects Glue schema conversion for unknown (feature unsupported). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
89e1ac1 to
019da08
Compare
b062b0e to
073000a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 073000a0a9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bac2002d27
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0a16bfa828
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
0a16bfa to
0144610
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0144610405
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
8f892c3 to
7e87c07
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e87c076ee
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .or_else(|| { | ||
| source.fields().iter().position(|source_field| { | ||
| source_field.name() == target_field.name() | ||
| }) | ||
| }) |
There was a problem hiding this comment.
When a projected struct contains an unknown child, the new recursive transformer runs for the entire struct; if an old nested field was deleted and a new field later reused its name with a different ID, the ID lookup correctly misses but this unconditional name fallback selects the deleted field's physical column. The scan then returns stale values for the newly added field instead of its initial default/null. Only use name/position fallback when the source nested fields do not carry IDs; when IDs are present, a missing target ID must remain missing.
Useful? React with 👍 / 👎.
7e87c07 to
e379217
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c7985d4a7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ensure_data_valid!( | ||
| field.initial_default.is_none() && field.write_default.is_none(), |
There was a problem hiding this comment.
When a struct/list/map field has a default containing a non-null value at a nested unknown position, this validator only checks defaults attached directly to the unknown child, so the schema passes validation. Programmatic metadata serialization then reaches the try_into_json(...).expect(...) path in NestedField serialization and panics when that nested value cannot be converted to unknown; inspect enclosing container defaults recursively and reject any non-null unknown value during schema construction.
Useful? React with 👍 / 👎.
| let props = WriterProperties::builder() | ||
| .set_content_defined_chunking(cdc) | ||
| .set_compression(compression) | ||
| .set_max_row_group_bytes(Some(table_props.parquet_row_group_size_bytes)) |
There was a problem hiding this comment.
When write.parquet.row-group-size-bytes=0, TableProperties::try_from accepts the value as a usize, but this call passes Some(0) to parquet-rs, whose set_max_row_group_bytes contract panics for zero. Thus ordinary table configuration can crash ParquetWriterBuilder::from_table_properties instead of returning its advertised Result; validate the value as positive before invoking the setter.
Useful? React with 👍 / 👎.
| self.client | ||
| .get_or_try_init(|| async { | ||
| RestClient::init(&self.user_config, self.resolve_auth_manager()?).await |
There was a problem hiding this comment.
In the advertised multi-session use case, callers can pass contexts with different identities or credentials, but every operation ignores that context and resolves this single catalog-wide RestClient and authentication session. Consequently all principals' requests use the same catalog token and configuration, so RestSessionCatalog cannot authenticate or isolate the different sessions it is intended to serve; derive or cache authentication per SessionContext rather than once per catalog.
AGENTS.md reference: AGENTS.md:L25-L30
Useful? React with 👍 / 👎.
| let compression = match codec { | ||
| CompressionCodec::None => Compression::UNCOMPRESSED, | ||
| CompressionCodec::Snappy => Compression::SNAPPY, | ||
| CompressionCodec::Lzo => Compression::LZO, |
There was a problem hiding this comment.
When write.parquet.compression-codec=lzo, property parsing advertises the value as supported and this branch successfully returns Compression::LZO, but parquet-rs only defines the enum value and has no LZO codec implementation, so the first non-empty write fails with an unsupported-codec error. Reject LZO from from_table_properties with FeatureUnsupported (or implement the codec) rather than returning a writer that cannot write any data.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a53f89acac
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let config = &self.user_config; | ||
| let auth_type = Self::auth_type(config); |
There was a problem hiding this comment.
When the client properties contain no OAuth hints but GET /v1/config supplies rest.auth.type=oauth2, a token, or a credential, this resolves NoopAuthManager from user_config before the handshake. Although the response is subsequently merged, NoopAuthManager::catalog_session ignores those merged authentication properties, so every later catalog request is sent unauthenticated. Resolve the catalog-session manager from the merged configuration while retaining the separately selected init session.
Useful? React with 👍 / 👎.
|
|
||
| // If the storage location is within the table prefix, files are already scoped to this | ||
| // table so there is no need to add database/table context to avoid collisions. | ||
| let context = if storage_location.starts_with(table_location) { |
There was a problem hiding this comment.
When an external object-storage path merely shares the table location's textual prefix, such as table s3://bucket/db/table with write.object-storage.path=s3://bucket/db/table-data, starts_with incorrectly classifies it as inside the table. The generator then omits the database/table context that external shared locations require, producing a layout that differs from the Java-compatible provider and can collide with another table using the same external location. Compare complete path segments rather than an arbitrary string prefix.
Useful? React with 👍 / 👎.
| if fields.is_empty() { | ||
| let nulls = arrow_buffer::NullBuffer::new_null(num_rows); | ||
| return Ok(Arc::new(StructArray::new_empty_fields( | ||
| num_rows, | ||
| Some(nulls), |
There was a problem hiding this comment.
When _partition is projected from an unpartitioned table, its unified type has no children and this validity buffer marks the struct null for every row. Iceberg declares _partition as a required struct, so an unpartitioned row should contain a valid empty struct rather than null; the current result changes queries such as _partition IS NULL and contradicts the metadata-column schema. Construct the empty StructArray without an all-null validity buffer and keep the output field non-nullable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fccee71386
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| sqlx::query(&migration_sql) | ||
| .execute(&pool) | ||
| .await | ||
| .map_err(from_sqlx_error)?; |
There was a problem hiding this comment.
When multiple catalog instances start concurrently with sql.schema-version=V1 against the same V0 database, they can both detect V0 before either executes this ALTER TABLE; the first succeeds, while the second receives a duplicate-column or schema-lock error and fails catalog initialization. Make the migration atomic/idempotent, or re-detect and accept V1 after the expected concurrent-migration error.
Useful? React with 👍 / 👎.
| match field.field_type.as_ref() { | ||
| Type::Primitive(PrimitiveType::Unknown) => {} |
There was a problem hiding this comment.
When a scan projects only an unknown field, this produces no physical leaf IDs, and get_arrow_projection_mask_with_field_ids consequently falls back to ProjectionMask::all() to recover the row count. Because an unknown field is absent from every compliant Parquet file, such a projection unnecessarily decodes every physical column in every file; use a row-count virtual column or another minimal row-count source instead of selecting all leaves.
Useful? React with 👍 / 👎.
| } else { | ||
| schema | ||
| .field_by_name_case_insensitive(column_name) | ||
| .map(|field| field.id) |
There was a problem hiding this comment.
When a valid schema contains names that differ only by case, such as id and ID, a case-insensitive scan now resolves through lowercase_name_to_id, whose construction silently overwrites one of those entries in nondeterministic HashMap iteration order. Even selecting an exact spelling with with_case_sensitive(false) can therefore read or filter the wrong field; detect multiple case-folded matches and return an ambiguity error instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c1a7f5e41
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Ok(ProjectionMask::all()) | ||
| // All requested columns are new or have no physical representation. An empty | ||
| // projection still preserves row counts for RecordBatchTransformer. | ||
| Ok(ProjectionMask::none(parquet_schema.num_columns())) |
There was a problem hiding this comment.
When a scan requests only fields absent from an older file, or only an unknown field, this now produces a zero-column RecordBatch. RecordBatchTransformer::transform_columns immediately returns an empty vector when columns.is_empty(), so constructing the non-empty target schema fails instead of materializing the requested default/null columns for each source row. Pass the batch's explicit row count through the transformation path rather than relying on columns[0].len().
Useful? React with 👍 / 👎.
| )); | ||
| } | ||
|
|
||
| let backoff = Self::build_backoff(table_props)?; |
There was a problem hiding this comment.
When update_table_properties() removes encryption.key-id from a table that already has encrypted snapshots, this newly permitted commit succeeds and persists the removal. On the next catalog load, EncryptionManager::from_table_metadata returns None, while ManifestListReader requires a manager for every existing snapshot with encryption_key_id, making the table's data unreadable. Reject removal of this property while encrypted snapshots exist, or preserve enough key-manager configuration to decrypt historical snapshots.
Useful? React with 👍 / 👎.
5c1a7f5 to
223d3f0
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 223d3f0cc7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| pub(crate) fn new(file_io: FileIO) -> Self { | ||
| Self { file_io } |
There was a problem hiding this comment.
When downstream code loads a ManifestFile using a standalone FileIO rather than a Table, this commit removes the public ManifestFile::load_manifest method but makes the replacement reader's only constructor crate-private. Such callers cannot construct ManifestReader at all, so upgrading causes a source break and removes the previously supported ability to read manifests outside a Table; make this constructor public or retain a forwarding load_manifest method.
Useful? React with 👍 / 👎.
223d3f0 to
f551818
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f551818c43
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
85b1ed3 to
007a95a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 007a95a42f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
007a95a to
60872f6
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 60872f6490
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
c4236cd to
11c8ef1
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 11c8ef16c7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let initial_default = parse_default(value.initial_default, &value.field_type)?; | ||
| let write_default = parse_default(value.write_default, &value.field_type)?; | ||
|
|
||
| Ok(NestedField { |
There was a problem hiding this comment.
When a consumer deserializes a NestedField directly with {"required":true,"type":"unknown"}, this conversion succeeds because it validates only defaults; the optionality check exists only in SchemaBuilder::build. Direct NestedField deserialization is explicitly supported and tested in this change, so it can expose an invalid v3 field that contradicts the null-only semantics of unknown. Apply the required/optional validation in this TryFrom path as well.
Useful? React with 👍 / 👎.
11c8ef1 to
02756bf
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 02756bfb89
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
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 is the first and independently mergeable layer of the Unknown-type work. It now contains only the Iceberg v3 logical type and format conversions:
PrimitiveType::Unknownparsing, serialization, schema/default validation, format-version checks, and public API support.Nulland Avronullto the Unknown type and preserves null-only values.This is the root of GitHub stack apache#8:
Are these changes tested?
cargo test -p iceberg --lib(1,588 passed)cargo test -p iceberg --lib(1,610 passed)make check-fmtmake check-clippygit diff --checkfor every split commitAI 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.