Skip to content

Commit f6d43a3

Browse files
committed
fix(rest): skip serializing unset optional fields in CreateTableRequest
Unset optional fields of CreateTableRequest (location, partition-spec, write-order, stage-create) and of the embedded UnboundPartitionSpec and UnboundPartitionField (spec-id, field-id) were serialized as explicit JSON nulls. Strict REST catalog servers such as Apache Polaris reject those create-table requests with 400 Bad Request. Skip serializing these fields when they are unset. Deserialization of explicit nulls is unchanged. Fixes #2135 Signed-off-by: rahulsmahadev <rahul.mahadev@databricks.com>
1 parent 5d9d0f5 commit f6d43a3

2 files changed

Lines changed: 151 additions & 0 deletions

File tree

crates/catalog/rest/src/types.rs

Lines changed: 117 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -251,14 +251,18 @@ pub struct CreateTableRequest {
251251
/// Name of the table to create
252252
pub name: String,
253253
/// Optional table location. If not provided, the server will choose a location.
254+
#[serde(skip_serializing_if = "Option::is_none")]
254255
pub location: Option<String>,
255256
/// Table schema
256257
pub schema: Schema,
257258
/// Optional partition specification. If not provided, the table will be unpartitioned.
259+
#[serde(skip_serializing_if = "Option::is_none")]
258260
pub partition_spec: Option<UnboundPartitionSpec>,
259261
/// Optional sort order for the table
262+
#[serde(skip_serializing_if = "Option::is_none")]
260263
pub write_order: Option<SortOrder>,
261264
/// Whether to stage the create for a transaction (true) or create immediately (false)
265+
#[serde(skip_serializing_if = "Option::is_none")]
262266
pub stage_create: Option<bool>,
263267
/// Optional properties to set on the table
264268
#[serde(default, skip_serializing_if = "HashMap::is_empty")]
@@ -355,4 +359,117 @@ mod tests {
355359
json_no_props
356360
);
357361
}
362+
363+
fn test_create_table_request_schema() -> Schema {
364+
serde_json::from_value(serde_json::json!({
365+
"type": "struct",
366+
"schema-id": 1,
367+
"fields": [
368+
{
369+
"id": 1,
370+
"name": "foo",
371+
"required": false,
372+
"type": "string"
373+
},
374+
{
375+
"id": 2,
376+
"name": "bar",
377+
"required": true,
378+
"type": "int"
379+
}
380+
],
381+
"identifier-field-ids": [2]
382+
}))
383+
.expect("Failed to deserialize test schema")
384+
}
385+
386+
#[test]
387+
fn test_create_table_request_minimal_serialization() {
388+
let request = CreateTableRequest {
389+
name: "tbl1".to_string(),
390+
location: None,
391+
schema: test_create_table_request_schema(),
392+
partition_spec: None,
393+
write_order: None,
394+
stage_create: None,
395+
properties: HashMap::new(),
396+
};
397+
398+
let serialized = serde_json::to_value(&request).expect("Serialization failed");
399+
let object = serialized.as_object().expect("Expected a JSON object");
400+
assert!(object.contains_key("name"));
401+
assert!(object.contains_key("schema"));
402+
assert!(!object.contains_key("location"));
403+
assert!(!object.contains_key("partition-spec"));
404+
assert!(!object.contains_key("write-order"));
405+
assert!(!object.contains_key("stage-create"));
406+
assert!(!object.contains_key("properties"));
407+
}
408+
409+
#[test]
410+
fn test_create_table_request_full_serialization() {
411+
let request: CreateTableRequest = serde_json::from_value(serde_json::json!({
412+
"name": "tbl1",
413+
"location": "s3://warehouse/tbl1",
414+
"schema": test_create_table_request_schema(),
415+
"partition-spec": {
416+
"spec-id": 1,
417+
"fields": [
418+
{
419+
"source-id": 2,
420+
"field-id": 1000,
421+
"name": "bar",
422+
"transform": "identity"
423+
}
424+
]
425+
},
426+
"write-order": {
427+
"order-id": 1,
428+
"fields": [
429+
{
430+
"transform": "identity",
431+
"source-id": 2,
432+
"direction": "asc",
433+
"null-order": "nulls-first"
434+
}
435+
]
436+
},
437+
"stage-create": true,
438+
"properties": {
439+
"owner": "test"
440+
}
441+
}))
442+
.expect("Deserialization failed");
443+
444+
let serialized = serde_json::to_value(&request).expect("Serialization failed");
445+
let object = serialized.as_object().expect("Expected a JSON object");
446+
assert_eq!(
447+
object.get("location"),
448+
Some(&serde_json::json!("s3://warehouse/tbl1"))
449+
);
450+
assert!(object.contains_key("partition-spec"));
451+
assert!(object.contains_key("write-order"));
452+
assert_eq!(object.get("stage-create"), Some(&serde_json::json!(true)));
453+
assert!(object.contains_key("properties"));
454+
}
455+
456+
#[test]
457+
fn test_create_table_request_deserialize_explicit_nulls() {
458+
let request: CreateTableRequest = serde_json::from_value(serde_json::json!({
459+
"name": "tbl1",
460+
"location": null,
461+
"schema": test_create_table_request_schema(),
462+
"partition-spec": null,
463+
"write-order": null,
464+
"stage-create": null
465+
}))
466+
.expect("Deserialization failed");
467+
468+
assert_eq!(request.name, "tbl1");
469+
assert_eq!(request.location, None);
470+
assert_eq!(request.partition_spec, None);
471+
assert_eq!(request.write_order, None);
472+
assert_eq!(request.stage_create, None);
473+
assert!(request.properties.is_empty());
474+
}
358475
}

crates/iceberg/src/spec/partition.rs

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -246,6 +246,7 @@ pub struct UnboundPartitionField {
246246
/// A partition field id that is used to identify a partition field and is unique within a partition spec.
247247
/// In v2 table metadata, it is unique across all partition specs.
248248
#[builder(default, setter(strip_option(fallback = field_id_opt)))]
249+
#[serde(skip_serializing_if = "Option::is_none")]
249250
pub field_id: Option<i32>,
250251
/// A partition name.
251252
pub name: String,
@@ -260,6 +261,7 @@ pub struct UnboundPartitionField {
260261
#[serde(rename_all = "kebab-case")]
261262
pub struct UnboundPartitionSpec {
262263
/// Identifier for PartitionSpec
264+
#[serde(skip_serializing_if = "Option::is_none")]
263265
pub(crate) spec_id: Option<i32>,
264266
/// Details of the partition spec
265267
pub(crate) fields: Vec<UnboundPartitionField>,
@@ -912,6 +914,38 @@ mod tests {
912914
assert_eq!(Transform::Day, partition_spec.fields[0].transform);
913915
}
914916

917+
#[test]
918+
fn test_unbound_partition_spec_serialization_skips_none_fields() {
919+
let spec = UnboundPartitionSpec::builder()
920+
.add_partition_field(4, "ts_day".to_string(), Transform::Day)
921+
.unwrap()
922+
.build();
923+
924+
let value = serde_json::to_value(&spec).unwrap();
925+
let object = value.as_object().unwrap();
926+
assert!(!object.contains_key("spec-id"));
927+
let field = object["fields"][0].as_object().unwrap();
928+
assert!(!field.contains_key("field-id"));
929+
930+
let value = serde_json::to_value(spec.with_spec_id(1)).unwrap();
931+
let object = value.as_object().unwrap();
932+
assert_eq!(Some(&serde_json::json!(1)), object.get("spec-id"));
933+
934+
// Explicit nulls must still deserialize to `None` for backwards
935+
// compatibility.
936+
let spec: UnboundPartitionSpec = serde_json::from_str(
937+
r#"{
938+
"spec-id": null,
939+
"fields": [
940+
{"source-id": 4, "name": "ts_day", "transform": "day", "field-id": null}
941+
]
942+
}"#,
943+
)
944+
.unwrap();
945+
assert_eq!(None, spec.spec_id);
946+
assert_eq!(None, spec.fields[0].field_id);
947+
}
948+
915949
#[test]
916950
fn test_new_unpartition() {
917951
let schema = Schema::builder()

0 commit comments

Comments
 (0)