liurenjie1024 commented on code in PR #272:
URL: https://github.com/apache/iceberg-rust/pull/272#discussion_r1529576423
##########
crates/iceberg/src/spec/table_metadata.rs:
##########
@@ -297,19 +298,37 @@ impl TableMetadataBuilder {
properties,
} = table_creation;
- if partition_spec.is_some() {
- return Err(Error::new(
- ErrorKind::FeatureUnsupported,
- "Can't create table with partition spec now",
- ));
- }
+ let partition_specs = match partition_spec {
+ Some(_) => {
+ return Err(Error::new(
+ ErrorKind::FeatureUnsupported,
+ "Can't create table with partition spec now",
+ ))
+ }
+ None => HashMap::from([(
+ 0,
+ Arc::new(PartitionSpec {
+ spec_id: 0,
+ fields: vec![],
+ }),
+ )]),
+ };
- if sort_order.is_some() {
- return Err(Error::new(
- ErrorKind::FeatureUnsupported,
- "Can't create table with sort order now",
- ));
- }
+ let sort_orders = match sort_order {
+ Some(_) => {
+ return Err(Error::new(
+ ErrorKind::FeatureUnsupported,
+ "Can't create table with sort order now",
+ ))
+ }
+ None => HashMap::from([(
+ 0,
+ Arc::new(SortOrder {
+ order_id: 0,
Review Comment:
Could we replace this with `DEFAULT_SORT_ORDER_ID`?
##########
crates/iceberg/src/spec/table_metadata.rs:
##########
@@ -727,14 +746,10 @@ pub(super) mod _serde {
.collect(),
default_spec_id: v.default_spec_id,
last_partition_id: v.last_partition_id,
- properties: if v.properties.is_empty() {
- None
- } else {
- Some(v.properties)
- },
+ properties: Some(v.properties),
current_snapshot_id: v.current_snapshot_id.or(Some(-1)),
snapshots: if v.snapshots.is_empty() {
- None
+ Some(vec![])
Review Comment:
This is interesting, the spec says `properties` and `snapshot` are both
optional, and [this
example](https://github.com/apache/iceberg/blob/3d929d459ab21efe313f42754b58b4392d547aee/core/src/test/resources/TableMetadataV2ValidMinimal.json)
says it doesn't require these fields.
##########
crates/iceberg/src/spec/table_metadata.rs:
##########
@@ -297,19 +298,37 @@ impl TableMetadataBuilder {
properties,
} = table_creation;
- if partition_spec.is_some() {
- return Err(Error::new(
- ErrorKind::FeatureUnsupported,
- "Can't create table with partition spec now",
- ));
- }
+ let partition_specs = match partition_spec {
+ Some(_) => {
+ return Err(Error::new(
+ ErrorKind::FeatureUnsupported,
+ "Can't create table with partition spec now",
+ ))
+ }
+ None => HashMap::from([(
+ 0,
Review Comment:
Could we replace these with `DEFAULT_SPEC_ID`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
