You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "ZENOTME (via GitHub)" <gi...@apache.org> on 2024/03/13 15:46:34 UTC
[PR] feat: init TableMetadataBuilder [iceberg-rust]
ZENOTME opened a new pull request, #262:
URL: https://github.com/apache/iceberg-rust/pull/262
This PR init TableMetadataBuilder. Complete #250
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: init TableMetadataBuilder [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 merged PR #262:
URL: https://github.com/apache/iceberg-rust/pull/262
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: init TableMetadataBuilder [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on code in PR #262:
URL: https://github.com/apache/iceberg-rust/pull/262#discussion_r1524213529
##########
crates/iceberg/src/spec/table_metadata.rs:
##########
@@ -275,6 +277,82 @@ impl TableMetadata {
}
}
+/// Manipulating table metadata.
+pub struct TableMetadataBuilder(TableMetadata);
+
+impl TableMetadataBuilder {
+ /// Creates a new table metadata builder from the given table metadata.
+ pub fn new(origin: TableMetadata) -> Self {
+ Self(origin)
+ }
+
+ /// Creates a new table metadata builder from the given table creation.
+ pub fn from_table_creation(table_creation: TableCreation) -> Result<Self> {
+ let TableCreation {
+ name: _,
+ location,
+ schema,
+ partition_spec,
+ sort_order,
+ properties,
+ } = table_creation;
+
+ if partition_spec.is_some() {
+ return Err(Error::new(
+ ErrorKind::FeatureUnsupported,
+ "Can't create table with partition spec now",
+ ));
+ }
+
+ if sort_order.is_some() {
+ return Err(Error::new(
+ ErrorKind::FeatureUnsupported,
+ "Can't create table with sort order now",
+ ));
+ }
+
+ let table_metadata = TableMetadata {
+ format_version: FormatVersion::V2,
+ table_uuid: Uuid::new_v4(),
+ location: location.ok_or_else(|| {
+ Error::new(
+ ErrorKind::DataInvalid,
+ "Can't create table without location",
+ )
+ })?,
+ last_sequence_number: 0,
+ last_updated_ms: Utc::now().timestamp_millis(),
+ last_column_id: schema.highest_field_id(),
+ current_schema_id: schema.schema_id(),
+ schemas: HashMap::from([(schema.schema_id(), Arc::new(schema))]),
+ partition_specs: Default::default(),
+ default_spec_id: 0,
+ last_partition_id: 0,
+ properties,
+ current_snapshot_id: None,
+ snapshots: Default::default(),
+ snapshot_log: vec![],
+ sort_orders: Default::default(),
+ metadata_log: vec![],
+ default_sort_order_id: 0,
+ refs: Default::default(),
+ };
+
+ Ok(Self(table_metadata))
+ }
+
+ /// Changes uuid of table metadata.
+ pub fn assign_uuid(mut self, uuid: Uuid) -> Result<Self> {
Review Comment:
Currently it's only used here, but it's also intended for user to construct table metadata, so I think it's ok to leave it `pub`
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: init TableMetadataBuilder [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on code in PR #262:
URL: https://github.com/apache/iceberg-rust/pull/262#discussion_r1524080929
##########
crates/iceberg/src/spec/table_metadata.rs:
##########
@@ -275,6 +277,78 @@ impl TableMetadata {
}
}
+/// Manipulating table metadata.
+pub struct TableMetadataBuilder(TableMetadata);
+
+impl TableMetadataBuilder {
+ /// Creates a new table metadata builder from the given table metadata.
+ pub fn new(origin: TableMetadata) -> Self {
+ Self(origin)
+ }
+
+ /// Creates a new table metadata builder from the given table creation.
+ pub fn from_table_creation(table_creation: TableCreation) -> Result<Self> {
+ let TableCreation {
+ name: _,
+ location,
+ schema,
+ partition_spec,
+ sort_order,
+ properties,
+ } = table_creation;
+
+ let table_metadata = TableMetadata {
+ format_version: FormatVersion::V2,
+ table_uuid: Uuid::new_v4(),
+ location: location.ok_or_else(|| {
+ Error::new(
+ ErrorKind::DataInvalid,
+ "Can't create table without location",
+ )
+ })?,
+ last_sequence_number: 0,
+ last_updated_ms: 0,
Review Comment:
This should be updated by current ts.
##########
crates/iceberg/src/spec/table_metadata.rs:
##########
@@ -275,6 +277,78 @@ impl TableMetadata {
}
}
+/// Manipulating table metadata.
+pub struct TableMetadataBuilder(TableMetadata);
+
+impl TableMetadataBuilder {
+ /// Creates a new table metadata builder from the given table metadata.
+ pub fn new(origin: TableMetadata) -> Self {
+ Self(origin)
+ }
+
+ /// Creates a new table metadata builder from the given table creation.
+ pub fn from_table_creation(table_creation: TableCreation) -> Result<Self> {
+ let TableCreation {
+ name: _,
+ location,
+ schema,
+ partition_spec,
+ sort_order,
+ properties,
+ } = table_creation;
+
+ let table_metadata = TableMetadata {
+ format_version: FormatVersion::V2,
+ table_uuid: Uuid::new_v4(),
+ location: location.ok_or_else(|| {
+ Error::new(
+ ErrorKind::DataInvalid,
+ "Can't create table without location",
+ )
+ })?,
+ last_sequence_number: 0,
+ last_updated_ms: 0,
+ last_column_id: 0,
Review Comment:
This should be schema's largest field id.
##########
crates/iceberg/src/spec/table_metadata.rs:
##########
@@ -275,6 +277,78 @@ impl TableMetadata {
}
}
+/// Manipulating table metadata.
+pub struct TableMetadataBuilder(TableMetadata);
+
+impl TableMetadataBuilder {
+ /// Creates a new table metadata builder from the given table metadata.
+ pub fn new(origin: TableMetadata) -> Self {
+ Self(origin)
+ }
+
+ /// Creates a new table metadata builder from the given table creation.
+ pub fn from_table_creation(table_creation: TableCreation) -> Result<Self> {
+ let TableCreation {
+ name: _,
+ location,
+ schema,
+ partition_spec,
+ sort_order,
+ properties,
+ } = table_creation;
+
+ let table_metadata = TableMetadata {
+ format_version: FormatVersion::V2,
+ table_uuid: Uuid::new_v4(),
+ location: location.ok_or_else(|| {
+ Error::new(
+ ErrorKind::DataInvalid,
+ "Can't create table without location",
+ )
+ })?,
+ last_sequence_number: 0,
+ last_updated_ms: 0,
+ last_column_id: 0,
+ schemas: HashMap::from([(0, Arc::new(schema))]),
+ current_schema_id: 0,
+ partition_specs: partition_spec
Review Comment:
This is incorrect, we should bind partition spec to schema.
##########
crates/iceberg/src/spec/table_metadata.rs:
##########
@@ -275,6 +277,78 @@ impl TableMetadata {
}
}
+/// Manipulating table metadata.
+pub struct TableMetadataBuilder(TableMetadata);
+
+impl TableMetadataBuilder {
+ /// Creates a new table metadata builder from the given table metadata.
+ pub fn new(origin: TableMetadata) -> Self {
+ Self(origin)
+ }
+
+ /// Creates a new table metadata builder from the given table creation.
+ pub fn from_table_creation(table_creation: TableCreation) -> Result<Self> {
+ let TableCreation {
+ name: _,
+ location,
+ schema,
+ partition_spec,
+ sort_order,
+ properties,
+ } = table_creation;
+
+ let table_metadata = TableMetadata {
+ format_version: FormatVersion::V2,
+ table_uuid: Uuid::new_v4(),
+ location: location.ok_or_else(|| {
+ Error::new(
+ ErrorKind::DataInvalid,
+ "Can't create table without location",
+ )
+ })?,
+ last_sequence_number: 0,
+ last_updated_ms: 0,
+ last_column_id: 0,
+ schemas: HashMap::from([(0, Arc::new(schema))]),
+ current_schema_id: 0,
+ partition_specs: partition_spec
+ .map(|x| {
+ let partition_spec = PartitionSpecRef::new(x.create_new());
+ HashMap::from([(partition_spec.spec_id, partition_spec)])
+ })
+ .unwrap_or_default(),
+ default_spec_id: 0,
+ last_partition_id: 0,
+ properties,
+ current_snapshot_id: None,
+ snapshots: Default::default(),
+ snapshot_log: vec![],
+ metadata_log: vec![],
+ sort_orders: sort_order
Review Comment:
We should bind to schema.
##########
crates/iceberg/src/spec/table_metadata.rs:
##########
@@ -275,6 +277,78 @@ impl TableMetadata {
}
}
+/// Manipulating table metadata.
+pub struct TableMetadataBuilder(TableMetadata);
+
+impl TableMetadataBuilder {
+ /// Creates a new table metadata builder from the given table metadata.
+ pub fn new(origin: TableMetadata) -> Self {
+ Self(origin)
+ }
+
+ /// Creates a new table metadata builder from the given table creation.
+ pub fn from_table_creation(table_creation: TableCreation) -> Result<Self> {
+ let TableCreation {
+ name: _,
+ location,
+ schema,
+ partition_spec,
+ sort_order,
+ properties,
+ } = table_creation;
+
+ let table_metadata = TableMetadata {
+ format_version: FormatVersion::V2,
+ table_uuid: Uuid::new_v4(),
+ location: location.ok_or_else(|| {
+ Error::new(
+ ErrorKind::DataInvalid,
+ "Can't create table without location",
+ )
+ })?,
+ last_sequence_number: 0,
+ last_updated_ms: 0,
+ last_column_id: 0,
+ schemas: HashMap::from([(0, Arc::new(schema))]),
+ current_schema_id: 0,
Review Comment:
Ditto, this should be schema's id.
##########
crates/iceberg/src/spec/table_metadata.rs:
##########
@@ -275,6 +277,78 @@ impl TableMetadata {
}
}
+/// Manipulating table metadata.
+pub struct TableMetadataBuilder(TableMetadata);
+
+impl TableMetadataBuilder {
+ /// Creates a new table metadata builder from the given table metadata.
+ pub fn new(origin: TableMetadata) -> Self {
+ Self(origin)
+ }
+
+ /// Creates a new table metadata builder from the given table creation.
+ pub fn from_table_creation(table_creation: TableCreation) -> Result<Self> {
+ let TableCreation {
+ name: _,
+ location,
+ schema,
+ partition_spec,
+ sort_order,
+ properties,
+ } = table_creation;
+
+ let table_metadata = TableMetadata {
+ format_version: FormatVersion::V2,
+ table_uuid: Uuid::new_v4(),
+ location: location.ok_or_else(|| {
+ Error::new(
+ ErrorKind::DataInvalid,
+ "Can't create table without location",
+ )
+ })?,
+ last_sequence_number: 0,
+ last_updated_ms: 0,
+ last_column_id: 0,
+ schemas: HashMap::from([(0, Arc::new(schema))]),
Review Comment:
The key should be schema 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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: init TableMetadataBuilder [iceberg-rust]
Posted by "marvinlanhenke (via GitHub)" <gi...@apache.org>.
marvinlanhenke commented on PR #262:
URL: https://github.com/apache/iceberg-rust/pull/262#issuecomment-1996606877
@ZENOTME
thanks so much for your work - this will definitely unblock me.
> We can left the binding to other pr, and forbid table creation with partition spec and sort order for now, it's up to you.
I think this is a good idea for now, so we can move on. LGTM! thanks again
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: init TableMetadataBuilder [iceberg-rust]
Posted by "marvinlanhenke (via GitHub)" <gi...@apache.org>.
marvinlanhenke commented on code in PR #262:
URL: https://github.com/apache/iceberg-rust/pull/262#discussion_r1524291618
##########
crates/iceberg/src/spec/table_metadata.rs:
##########
@@ -275,6 +277,82 @@ impl TableMetadata {
}
}
+/// Manipulating table metadata.
+pub struct TableMetadataBuilder(TableMetadata);
+
+impl TableMetadataBuilder {
+ /// Creates a new table metadata builder from the given table metadata.
+ pub fn new(origin: TableMetadata) -> Self {
+ Self(origin)
+ }
+
+ /// Creates a new table metadata builder from the given table creation.
+ pub fn from_table_creation(table_creation: TableCreation) -> Result<Self> {
Review Comment:
I really like this; makes me life a lot easier in catalog/hms
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: init TableMetadataBuilder [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on PR #262:
URL: https://github.com/apache/iceberg-rust/pull/262#issuecomment-1996240458
cc @marvinlanhenke @Fokko @Xuanwo PTAL
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: init TableMetadataBuilder [iceberg-rust]
Posted by "ZENOTME (via GitHub)" <gi...@apache.org>.
ZENOTME commented on PR #262:
URL: https://github.com/apache/iceberg-rust/pull/262#issuecomment-1996316028
> We can left the binding to other pr, and forbid table creation with partition spec and sort order for now, it's up to you.
Let's forbid them now. I think I'm not clear with these two processes now, so let's delay them to later PR. After I complete them, I will fix this. Other fixed.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: init TableMetadataBuilder [iceberg-rust]
Posted by "ZENOTME (via GitHub)" <gi...@apache.org>.
ZENOTME commented on code in PR #262:
URL: https://github.com/apache/iceberg-rust/pull/262#discussion_r1524159851
##########
crates/iceberg/src/spec/table_metadata.rs:
##########
@@ -275,6 +277,82 @@ impl TableMetadata {
}
}
+/// Manipulating table metadata.
+pub struct TableMetadataBuilder(TableMetadata);
+
+impl TableMetadataBuilder {
+ /// Creates a new table metadata builder from the given table metadata.
+ pub fn new(origin: TableMetadata) -> Self {
+ Self(origin)
+ }
+
+ /// Creates a new table metadata builder from the given table creation.
+ pub fn from_table_creation(table_creation: TableCreation) -> Result<Self> {
+ let TableCreation {
+ name: _,
+ location,
+ schema,
+ partition_spec,
+ sort_order,
+ properties,
+ } = table_creation;
+
+ if partition_spec.is_some() {
+ return Err(Error::new(
+ ErrorKind::FeatureUnsupported,
+ "Can't create table with partition spec now",
+ ));
+ }
+
+ if sort_order.is_some() {
+ return Err(Error::new(
+ ErrorKind::FeatureUnsupported,
+ "Can't create table with sort order now",
+ ));
+ }
+
+ let table_metadata = TableMetadata {
+ format_version: FormatVersion::V2,
+ table_uuid: Uuid::new_v4(),
+ location: location.ok_or_else(|| {
+ Error::new(
+ ErrorKind::DataInvalid,
+ "Can't create table without location",
+ )
+ })?,
+ last_sequence_number: 0,
+ last_updated_ms: Utc::now().timestamp_millis(),
+ last_column_id: schema.highest_field_id(),
+ current_schema_id: schema.schema_id(),
+ schemas: HashMap::from([(schema.schema_id(), Arc::new(schema))]),
+ partition_specs: Default::default(),
+ default_spec_id: 0,
+ last_partition_id: 0,
+ properties,
+ current_snapshot_id: None,
+ snapshots: Default::default(),
+ snapshot_log: vec![],
+ sort_orders: Default::default(),
+ metadata_log: vec![],
+ default_sort_order_id: 0,
+ refs: Default::default(),
+ };
+
+ Ok(Self(table_metadata))
+ }
+
+ /// Changes uuid of table metadata.
+ pub fn assign_uuid(mut self, uuid: Uuid) -> Result<Self> {
Review Comment:
If it only used in TableUpdate.apply, we can let this be `pub(crate)`.🤔
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: init TableMetadataBuilder [iceberg-rust]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on code in PR #262:
URL: https://github.com/apache/iceberg-rust/pull/262#discussion_r1524157344
##########
crates/iceberg/src/spec/partition.rs:
##########
@@ -133,6 +133,25 @@ impl UnboundPartitionSpec {
pub fn builder() -> UnboundPartitionSpecBuilder {
UnboundPartitionSpecBuilder::default()
}
+
+ /// Create a [`PartitionSpec`] for a new table. So it don't need
+ /// to specify the partition id for each field.
+ pub fn create_new(self) -> PartitionSpec {
Review Comment:
This is unused, show we remove this?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org