liurenjie1024 commented on code in PR #1087:
URL: https://github.com/apache/iceberg-rust/pull/1087#discussion_r2004627669
##########
crates/iceberg/src/spec/table_metadata.rs:
##########
@@ -942,9 +944,11 @@ pub(super) mod _serde {
))
})
.or_else(|| {
- Some(value.schema.try_into().map(|schema: Schema| {
Review Comment:
I would suggest to make the logic more clear here:
1. We priotize `schemas + schema_id` for current schema
2. If 1 not found, we should use `schema` field
3. If neither found, we throw an error.
##########
crates/iceberg/src/spec/table_metadata.rs:
##########
@@ -725,12 +725,14 @@ pub(super) mod _serde {
pub location: String,
pub last_updated_ms: i64,
pub last_column_id: i32,
- pub schema: SchemaV1,
+ #[serde(skip_serializing_if = "Option::is_none")]
+ pub schema: Option<SchemaV1>,
Review Comment:
Following the discussion in [dev thread
list](https://lists.apache.org/thread/npgwpgcwgft9h85ozjrds4yh2yrfy0vg), we
should following a `read liberally and write strictly` approach. So we should
do a check when converting `TableMetadata` to `TableMetadataV1` that this field
should not be `None`.
##########
crates/iceberg/src/spec/table_metadata.rs:
##########
@@ -725,12 +725,14 @@ pub(super) mod _serde {
pub location: String,
pub last_updated_ms: i64,
pub last_column_id: i32,
- pub schema: SchemaV1,
+ #[serde(skip_serializing_if = "Option::is_none")]
+ pub schema: Option<SchemaV1>,
#[serde(skip_serializing_if = "Option::is_none")]
pub schemas: Option<Vec<SchemaV1>>,
#[serde(skip_serializing_if = "Option::is_none")]
pub current_schema_id: Option<i32>,
- pub partition_spec: Vec<PartitionField>,
+ #[serde(skip_serializing_if = "Option::is_none")]
+ pub partition_spec: Option<Vec<PartitionField>>,
Review Comment:
Ditto.
##########
crates/iceberg/src/spec/table_metadata.rs:
##########
@@ -966,10 +970,15 @@ pub(super) mod _serde {
let partition_specs = match value.partition_specs {
Some(partition_specs) => partition_specs,
- None => vec![PartitionSpec::builder(current_schema.clone())
- .with_spec_id(DEFAULT_PARTITION_SPEC_ID)
-
.add_unbound_fields(value.partition_spec.into_iter().map(|f| f.into_unbound()))?
- .build()?],
+ None => match value.partition_spec {
+ Some(partition_spec) =>
vec