feat: Implement TableRequirement checks #689
Conversation
| /// 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, |
There was a problem hiding this comment.
Introduced type changes to match TableMetadata and TableUpdate
liurenjie1024
left a comment
liurenjie1024
left a comment
There was a problem hiding this comment.
Thanks @c-thiel for this pr! Left some minor points to discuss, others look great!
crates/iceberg/src/catalog/mod.rs
Outdated
| /// Current schema id of the table to assert. | ||
| #[serde(rename = "current-schema-id")] | ||
| current_schema_id: i64, | ||
| current_schema_id: i32, |
crates/iceberg/src/error.rs
Outdated
| /// This error is returned when given iceberg feature is not supported. | ||
| FeatureUnsupported, | ||
| /// A requirement check failed. | ||
| RequirementFailed, |
There was a problem hiding this 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
There was a problem hiding this comment.
Java is a bit more detailed here. I am also fine with keeping DataInvalid, but would in that case add in the message that the data is actually not invalid, but just the requirement not met.
There was a problem hiding this comment.
I changed it in the latest commit, let me know what you think!
crates/iceberg/src/catalog/mod.rs
Outdated
| 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<()> { |
There was a problem hiding this comment.
| pub fn check(&self, metadata: &TableMetadata, exists: bool) -> Result<()> { | |
| pub fn check(&self, metadata: Option<&TableMetadata>) -> Result<()> { |
How about use Option here?
There was a problem hiding this comment.
You are right, this is better.
My motivation was that the Option approach is not able to combine any other Requirement with the TableDoesNotExist Requirement. Paradoxically tables can actually NotExist even if Metadata is available: The staged-create create table does exactly this - prepare metadata while the table technically doesn't exist yet.
However, I checked Java, and it doesn't allow the combination in question, so we don't need to prepare for it.
https://github.com/apache/iceberg/blob/fff9ec3bbc322080da6363b657415b039c0e92a0/core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java#L361C1-L376C4
crates/iceberg/src/catalog/mod.rs
Outdated
| "Current schema id does not match", | ||
| ) | ||
| .with_context("expected", current_schema_id.to_string()) | ||
| .with_context("found", metadata.current_schema_id.to_string())); |
crates/iceberg/src/catalog/mod.rs
Outdated
| ErrorKind::RequirementFailed, | ||
| "Default sort order id does not match", | ||
| ) | ||
| .with_context("expected", default_sort_order_id.to_string()) |
crates/iceberg/src/catalog/mod.rs
Outdated
| "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())); |
crates/iceberg/src/catalog/mod.rs
Outdated
| if snapshot_ref.snapshot_id() != *snapshot_id { | ||
| return Err(Error::new( | ||
| ErrorKind::RequirementFailed, | ||
| format!("Branch or tag `{}` has changed", r#ref), |
There was a problem hiding this comment.
| format!("Branch or tag `{}` has changed", r#ref), | |
| format!("Branch or tag `{}`'s snapshot has changed", r#ref), |
|
@liurenjie1024 ready for another round! |
liurenjie1024
left a comment
liurenjie1024
left a comment
There was a problem hiding this comment.
Thanks @c-thiel for this pr, LGTM!
* Impelment TableRequirement check * Address comments
* Impelment TableRequirement check * Address comments
2 participants
No description provided.