Skip to content

fix(rest): omit CreateTableRequest.location when None - #165

Open
forrestlinfeng wants to merge 1 commit into
risingwavelabs:dev_rebase_main_20260303from
forrestlinfeng:fix/rest-skip-null-location-in-create-table
Open

forrestlinfeng wants to merge 1 commit into
risingwavelabs:dev_rebase_main_20260303from
forrestlinfeng:fix/rest-skip-null-location-in-create-table

Conversation

@forrestlinfeng

Copy link
Copy Markdown

Which issue does this PR close?

No issue filed — reporting and fixing directly. Happy to open one if preferred.

What changes are included in this PR?

CreateTableRequest.location is declared as Option<String> but lacks skip_serializing_if, so serde_json emits "location": null when the caller passes None. Per the Iceberg REST spec the field is optional and should be omitted when unset.

Several REST-compatible catalog servers (notably AWS Glue REST) reject a present-but-null location and never fall back to a server-side default (e.g. the Glue database's LocationUri). The result is Location information cannot be null errors that cannot be resolved from the server side — the database may already have a LocationUri set, but it's never consulted because the request actively provides location: null.

This PR adds #[serde(default, skip_serializing_if = "Option::is_none")] to the location field, matching how properties is already handled on the same struct and matching spec semantics for optional fields.

     /// Optional table location. If not provided, the server will choose a location.
+    #[serde(default, skip_serializing_if = "Option::is_none")]
     pub location: Option<String>,

Scope

Intentionally minimal — only location is touched. The same Option<T> pattern without skip_serializing_if exists on partition_spec, write_order, and stage_create, and is likely worth the same treatment in a follow-up, but those haven't been observed to trigger server-side rejections in practice.

Compatibility

This is a backwards-compatible serialization change:

  • Before: {"name": "t", "location": null, "schema": {...}, ...}
  • After: {"name": "t", "schema": {...}, ...}

Spec-compliant REST servers MUST treat a missing optional field the same as an explicit null, so well-behaved servers (Polaris, Tabular, Lakekeeper, etc.) are unaffected. Strict servers like Glue REST start working.

Are these changes tested?

Yes — added a unit test types::tests::test_create_table_request_omits_null_location that constructs a CreateTableRequest { location: None, .. } and asserts the serialized JSON has no "location" key. Existing catalog::tests::test_create_table and test_create_table_409 still pass.

$ cargo test -p iceberg-catalog-rest --lib types:: catalog::tests::test_create_table
running 2 tests
test types::tests::test_create_table_request_omits_null_location ... ok
test types::tests::test_namespace_response_serde ... ok
test result: ok. 2 passed; 0 failed
running 2 tests
test catalog::tests::test_create_table_409 ... ok
test catalog::tests::test_create_table ... ok
test result: ok. 2 passed; 0 failed

Previously `location: Option<String>` was serialized as `"location": null`
when unset, because the field had no `skip_serializing_if`. Some REST-
compatible catalogs (notably AWS Glue REST) reject a present-but-null
`location` even though the Iceberg REST spec marks the field as optional,
expecting it to be omitted so they can fall back to a server-side default
(e.g. the Glue database's `LocationUri`).

Add `#[serde(default, skip_serializing_if = "Option::is_none")]` so the
field is omitted when None, matching how `properties` is already handled
and matching the spec semantics for optional fields. Also adds a unit test.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant