liurenjie1024 commented on code in PR #689:
URL: https://github.com/apache/iceberg-rust/pull/689#discussion_r1834381560
##########
crates/iceberg/src/error.rs:
##########
@@ -44,6 +44,8 @@ pub enum ErrorKind {
///
/// This error is returned when given iceberg feature is not supported.
FeatureUnsupported,
+ /// A requirement check failed.
+ RequirementFailed,
Review Comment:
Should we actually add this? I think `DataInvalid` would be enough? We
should be cautious when adding new error kind, if user can't take different
actions from existing error kind. cc @Xuanwo
##########
crates/iceberg/src/catalog/mod.rs:
##########
@@ -453,6 +453,115 @@ impl TableUpdate {
}
}
+impl TableRequirement {
+ /// Check that the requirement is met by the table metadata.
+ /// If the requirement is not met, an appropriate error is returned.
+ pub fn check(&self, metadata: &TableMetadata, exists: bool) -> Result<()> {
Review Comment:
```suggestion
pub fn check(&self, metadata: Option<&TableMetadata>) -> Result<()> {
```
How about use `Option` here?
##########
crates/iceberg/src/catalog/mod.rs:
##########
@@ -453,6 +453,115 @@ impl TableUpdate {
}
}
+impl TableRequirement {
+ /// Check that the requirement is met by the table metadata.
+ /// If the requirement is not met, an appropriate error is returned.
+ pub fn check(&self, metadata: &TableMetadata, exists: bool) -> Result<()> {
+ match self {
+ TableRequirement::NotExist => {
+ if exists {
+ return Err(Error::new(
+ ErrorKind::RequirementFailed,
+ format!("Table with id {} already exists",
metadata.uuid()),
+ ));
+ }
+ }
+ TableRequirement::UuidMatch { uuid } => {
+ if &metadata.uuid() != uuid {
+ return Err(Error::new(
+ ErrorKind::RequirementFailed,
+ "Table UUID does not match",
+ )
+ .with_context("expected", *uuid)
+ .with_context("found", metadata.uuid()));
+ }
+ }
+ TableRequirement::CurrentSchemaIdMatch { current_schema_id } => {
+ // ToDo: Harmonize the types of current_schema_id
+ if metadata.current_schema_id != *current_schema_id {
+ return Err(Error::new(
+ ErrorKind::RequirementFailed,
+ "Current schema id does not match",
+ )
+ .with_context("expected", current_schema_id.to_string())
+ .with_context("found",
metadata.current_schema_id.to_string()));
+ }
+ }
+ TableRequirement::DefaultSortOrderIdMatch {
+ default_sort_order_id,
+ } => {
+ if metadata.default_sort_order().order_id !=
*default_sort_order_id {
+ return Err(Error::new(
+ ErrorKind::RequirementFailed,
+ "Default sort order id does not match",
+ )
+ .with_context("expected",
default_sort_order_id.to_string())
+ .with_context("found",
metadata.default_sort_order().order_id.to_string()));
+ }
+ }
+ TableRequirement::RefSnapshotIdMatch { r#ref, snapshot_id } => {
+ let snapshot_ref = metadata.snapshot_for_ref(r#ref);
+ if let Some(snapshot_id) = snapshot_id {
+ let snapshot_ref = snapshot_ref.ok_or(Error::new(
+ ErrorKind::RequirementFailed,
+ format!("Branch or tag `{}` not found", r#ref),
+ ))?;
+ if snapshot_ref.snapshot_id() != *snapshot_id {
+ return Err(Error::new(
+ ErrorKind::RequirementFailed,
+ format!("Branch or tag `{}` has changed", r#ref),
Review Comment:
```suggestion
format!("Branch or tag `{}`'s snapshot has
changed", r#ref),
```
##########
crates/iceberg/src/catalog/mod.rs:
##########
@@ -453,6 +453,115 @@ impl TableUpdate {
}
}
+impl TableRequirement {
+ /// Check that the requirement is met by the table metadata.
+ /// If the requirement is not met, an appropriate error is returned.
+ pub fn check(&self, metadata: &TableMetadata, exists: bool) -> Result<()> {
+ match self {
+ TableRequirement::NotExist => {
+ if exists {
+ return Err(Error::new(
+ ErrorKind::RequirementFailed,
+ format!("Table with id {} already exists",
metadata.uuid()),
+ ));
+ }
+ }
+ TableRequirement::UuidMatch { uuid } => {
+ if &metadata.uuid() != uuid {
+ return Err(Error::new(
+ ErrorKind::RequirementFailed,
+ "Table UUID does not match",
+ )
+ .with_context("expected", *uuid)
+ .with_context("found", metadata.uuid()));
+ }
+ }
+ TableRequirement::CurrentSchemaIdMatch { current_schema_id } => {
+ // ToDo: Harmonize the types of current_schema_id
+ if metadata.current_schema_id != *current_schema_id {
+ return Err(Error::new(
+ ErrorKind::RequirementFailed,
+ "Current schema id does not match",
+ )
+ .with_context("expected", current_schema_id.to_string())
+ .with_context("found",
metadata.current_schema_id.to_string()));
Review Comment:
Ditto.
##########
crates/iceberg/src/catalog/mod.rs:
##########
@@ -312,29 +312,29 @@ pub enum TableRequirement {
LastAssignedFieldIdMatch {
/// The last assigned field id of the table to assert.
#[serde(rename = "last-assigned-field-id")]
- last_assigned_field_id: i64,
+ last_assigned_field_id: i32,
},
/// The table's current schema id must match the requirement.
#[serde(rename = "assert-current-schema-id")]
CurrentSchemaIdMatch {
/// Current schema id of the table to assert.
#[serde(rename = "current-schema-id")]
- current_schema_id: i64,
+ current_schema_id: i32,
Review Comment:
Use `SchemaId` here?
##########
crates/iceberg/src/catalog/mod.rs:
##########
@@ -453,6 +453,115 @@ impl TableUpdate {
}
}
+impl TableRequirement {
+ /// Check that the requirement is met by the table metadata.
+ /// If the requirement is not met, an appropriate error is returned.
+ pub fn check(&self, metadata: &TableMetadata, exists: bool) -> Result<()> {
+ match self {
+ TableRequirement::NotExist => {
+ if exists {
+ return Err(Error::new(
+ ErrorKind::RequirementFailed,
+ format!("Table with id {} already exists",
metadata.uuid()),
+ ));
+ }
+ }
+ TableRequirement::UuidMatch { uuid } => {
+ if &metadata.uuid() != uuid {
+ return Err(Error::new(
+ ErrorKind::RequirementFailed,
+ "Table UUID does not match",
+ )
+ .with_context("expected", *uuid)
+ .with_context("found", metadata.uuid()));
+ }
+ }
+ TableRequirement::CurrentSchemaIdMatch { current_schema_id } => {
+ // ToDo: Harmonize the types of current_schema_id
+ if metadata.current_schema_id != *current_schema_id {
+ return Err(Error::new(
+ ErrorKind::RequirementFailed,
+ "Current schema id does not match",
+ )
+ .with_context("expected", current_schema_id.to_string())
Review Comment:
nit: There is no need to call `to_string`
##########
crates/iceberg/src/catalog/mod.rs:
##########
@@ -453,6 +453,115 @@ impl TableUpdate {
}
}
+impl TableRequirement {
+ /// Check that the requirement is met by the table metadata.
+ /// If the requirement is not met, an appropriate error is returned.
+ pub fn check(&self, metadata: &TableMetadata, exists: bool) -> Result<()> {
+ match self {
+ TableRequirement::NotExist => {
+ if exists {
+ return Err(Error::new(
+ ErrorKind::RequirementFailed,
+ format!("Table with id {} already exists",
metadata.uuid()),
+ ));
+ }
+ }
+ TableRequirement::UuidMatch { uuid } => {
+ if &metadata.uuid() != uuid {
+ return Err(Error::new(
+ ErrorKind::RequirementFailed,
+ "Table UUID does not match",
+ )
+ .with_context("expected", *uuid)
+ .with_context("found", metadata.uuid()));
+ }
+ }
+ TableRequirement::CurrentSchemaIdMatch { current_schema_id } => {
+ // ToDo: Harmonize the types of current_schema_id
+ if metadata.current_schema_id != *current_schema_id {
+ return Err(Error::new(
+ ErrorKind::RequirementFailed,
+ "Current schema id does not match",
+ )
+ .with_context("expected", current_schema_id.to_string())
+ .with_context("found",
metadata.current_schema_id.to_string()));
+ }
+ }
+ TableRequirement::DefaultSortOrderIdMatch {
+ default_sort_order_id,
+ } => {
+ if metadata.default_sort_order().order_id !=
*default_sort_order_id {
+ return Err(Error::new(
+ ErrorKind::RequirementFailed,
+ "Default sort order id does not match",
+ )
+ .with_context("expected",
default_sort_order_id.to_string())
Review Comment:
Ditto.
##########
crates/iceberg/src/catalog/mod.rs:
##########
@@ -453,6 +453,115 @@ impl TableUpdate {
}
}
+impl TableRequirement {
+ /// Check that the requirement is met by the table metadata.
+ /// If the requirement is not met, an appropriate error is returned.
+ pub fn check(&self, metadata: &TableMetadata, exists: bool) -> Result<()> {
+ match self {
+ TableRequirement::NotExist => {
+ if exists {
+ return Err(Error::new(
+ ErrorKind::RequirementFailed,
+ format!("Table with id {} already exists",
metadata.uuid()),
+ ));
+ }
+ }
+ TableRequirement::UuidMatch { uuid } => {
+ if &metadata.uuid() != uuid {
+ return Err(Error::new(
+ ErrorKind::RequirementFailed,
+ "Table UUID does not match",
+ )
+ .with_context("expected", *uuid)
+ .with_context("found", metadata.uuid()));
+ }
+ }
+ TableRequirement::CurrentSchemaIdMatch { current_schema_id } => {
+ // ToDo: Harmonize the types of current_schema_id
+ if metadata.current_schema_id != *current_schema_id {
+ return Err(Error::new(
+ ErrorKind::RequirementFailed,
+ "Current schema id does not match",
+ )
+ .with_context("expected", current_schema_id.to_string())
+ .with_context("found",
metadata.current_schema_id.to_string()));
+ }
+ }
+ TableRequirement::DefaultSortOrderIdMatch {
+ default_sort_order_id,
+ } => {
+ if metadata.default_sort_order().order_id !=
*default_sort_order_id {
+ return Err(Error::new(
+ ErrorKind::RequirementFailed,
+ "Default sort order id does not match",
+ )
+ .with_context("expected",
default_sort_order_id.to_string())
+ .with_context("found",
metadata.default_sort_order().order_id.to_string()));
Review Comment:
Ditto
--
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
