Skip to content

Commit a675031

Browse files
authored
fix: Use CatalogCommitConflicts for TableRequirement (#1532)
## Which issue does this PR close? - Closes #1531 ## What changes are included in this PR? - Use `ErrorKind::CatalogCommitConflicts` for `TableRequirement::check` - When metadata is None, returns `ErrorKind::TableNotFound` - Minor trimmings ## Are these changes tested? No
1 parent ddb2ab9 commit a675031

File tree

2 files changed

+39
-30
lines changed

2 files changed

+39
-30
lines changed

crates/iceberg/src/catalog/mod.rs

Lines changed: 37 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -597,111 +597,120 @@ impl TableRequirement {
597597
match self {
598598
TableRequirement::NotExist => {
599599
return Err(Error::new(
600-
ErrorKind::DataInvalid,
600+
ErrorKind::CatalogCommitConflicts,
601601
format!(
602602
"Requirement failed: Table with id {} already exists",
603603
metadata.uuid()
604604
),
605-
));
605+
)
606+
.with_retryable(true));
606607
}
607608
TableRequirement::UuidMatch { uuid } => {
608609
if &metadata.uuid() != uuid {
609610
return Err(Error::new(
610-
ErrorKind::DataInvalid,
611+
ErrorKind::CatalogCommitConflicts,
611612
"Requirement failed: Table UUID does not match",
612613
)
613614
.with_context("expected", *uuid)
614-
.with_context("found", metadata.uuid()));
615+
.with_context("found", metadata.uuid())
616+
.with_retryable(true));
615617
}
616618
}
617619
TableRequirement::CurrentSchemaIdMatch { current_schema_id } => {
618620
// ToDo: Harmonize the types of current_schema_id
619621
if metadata.current_schema_id != *current_schema_id {
620622
return Err(Error::new(
621-
ErrorKind::DataInvalid,
623+
ErrorKind::CatalogCommitConflicts,
622624
"Requirement failed: Current schema id does not match",
623625
)
624626
.with_context("expected", current_schema_id.to_string())
625-
.with_context("found", metadata.current_schema_id.to_string()));
627+
.with_context("found", metadata.current_schema_id.to_string())
628+
.with_retryable(true));
626629
}
627630
}
628631
TableRequirement::DefaultSortOrderIdMatch {
629632
default_sort_order_id,
630633
} => {
631634
if metadata.default_sort_order().order_id != *default_sort_order_id {
632635
return Err(Error::new(
633-
ErrorKind::DataInvalid,
636+
ErrorKind::CatalogCommitConflicts,
634637
"Requirement failed: Default sort order id does not match",
635638
)
636639
.with_context("expected", default_sort_order_id.to_string())
637-
.with_context(
638-
"found",
639-
metadata.default_sort_order().order_id.to_string(),
640-
));
640+
.with_context("found", metadata.default_sort_order().order_id.to_string())
641+
.with_retryable(true));
641642
}
642643
}
643644
TableRequirement::RefSnapshotIdMatch { r#ref, snapshot_id } => {
644645
let snapshot_ref = metadata.snapshot_for_ref(r#ref);
645646
if let Some(snapshot_id) = snapshot_id {
646-
let snapshot_ref = snapshot_ref.ok_or(Error::new(
647-
ErrorKind::DataInvalid,
648-
format!("Requirement failed: Branch or tag `{}` not found", r#ref),
649-
))?;
647+
let snapshot_ref = snapshot_ref.ok_or(
648+
Error::new(
649+
ErrorKind::CatalogCommitConflicts,
650+
format!("Requirement failed: Branch or tag `{}` not found", r#ref),
651+
)
652+
.with_retryable(true),
653+
)?;
650654
if snapshot_ref.snapshot_id() != *snapshot_id {
651655
return Err(Error::new(
652-
ErrorKind::DataInvalid,
656+
ErrorKind::CatalogCommitConflicts,
653657
format!(
654658
"Requirement failed: Branch or tag `{}`'s snapshot has changed",
655659
r#ref
656660
),
657661
)
658662
.with_context("expected", snapshot_id.to_string())
659-
.with_context("found", snapshot_ref.snapshot_id().to_string()));
663+
.with_context("found", snapshot_ref.snapshot_id().to_string())
664+
.with_retryable(true));
660665
}
661666
} else if snapshot_ref.is_some() {
662667
// a null snapshot ID means the ref should not exist already
663668
return Err(Error::new(
664-
ErrorKind::DataInvalid,
669+
ErrorKind::CatalogCommitConflicts,
665670
format!(
666671
"Requirement failed: Branch or tag `{}` already exists",
667672
r#ref
668673
),
669-
));
674+
)
675+
.with_retryable(true));
670676
}
671677
}
672678
TableRequirement::DefaultSpecIdMatch { default_spec_id } => {
673679
// ToDo: Harmonize the types of default_spec_id
674680
if metadata.default_partition_spec_id() != *default_spec_id {
675681
return Err(Error::new(
676-
ErrorKind::DataInvalid,
682+
ErrorKind::CatalogCommitConflicts,
677683
"Requirement failed: Default partition spec id does not match",
678684
)
679685
.with_context("expected", default_spec_id.to_string())
680-
.with_context("found", metadata.default_partition_spec_id().to_string()));
686+
.with_context("found", metadata.default_partition_spec_id().to_string())
687+
.with_retryable(true));
681688
}
682689
}
683690
TableRequirement::LastAssignedPartitionIdMatch {
684691
last_assigned_partition_id,
685692
} => {
686693
if metadata.last_partition_id != *last_assigned_partition_id {
687694
return Err(Error::new(
688-
ErrorKind::DataInvalid,
695+
ErrorKind::CatalogCommitConflicts,
689696
"Requirement failed: Last assigned partition id does not match",
690697
)
691698
.with_context("expected", last_assigned_partition_id.to_string())
692-
.with_context("found", metadata.last_partition_id.to_string()));
699+
.with_context("found", metadata.last_partition_id.to_string())
700+
.with_retryable(true));
693701
}
694702
}
695703
TableRequirement::LastAssignedFieldIdMatch {
696704
last_assigned_field_id,
697705
} => {
698706
if &metadata.last_column_id != last_assigned_field_id {
699707
return Err(Error::new(
700-
ErrorKind::DataInvalid,
708+
ErrorKind::CatalogCommitConflicts,
701709
"Requirement failed: Last assigned field id does not match",
702710
)
703711
.with_context("expected", last_assigned_field_id.to_string())
704-
.with_context("found", metadata.last_column_id.to_string()));
712+
.with_context("found", metadata.last_column_id.to_string())
713+
.with_retryable(true));
705714
}
706715
}
707716
};
@@ -710,7 +719,7 @@ impl TableRequirement {
710719
TableRequirement::NotExist => {}
711720
_ => {
712721
return Err(Error::new(
713-
ErrorKind::DataInvalid,
722+
ErrorKind::TableNotFound,
714723
"Requirement failed: Table does not exist",
715724
));
716725
}
@@ -814,7 +823,7 @@ pub enum ViewUpdate {
814823
#[serde(rename_all = "kebab-case")]
815824
AssignUuid {
816825
/// The new UUID to assign.
817-
uuid: uuid::Uuid,
826+
uuid: Uuid,
818827
},
819828
/// Upgrade view's format version
820829
#[serde(rename_all = "kebab-case")]
@@ -1092,7 +1101,7 @@ mod tests {
10921101
.unwrap()
10931102
.metadata;
10941103

1095-
// Ref exists and should matches
1104+
// Ref exists and should match
10961105
let requirement = TableRequirement::RefSnapshotIdMatch {
10971106
r#ref: "main".to_string(),
10981107
snapshot_id: Some(3051729675574597004),

crates/iceberg/src/error.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ pub enum ErrorKind {
5353
/// Iceberg namespace already exists at creation.
5454
NamespaceNotFound,
5555

56-
/// Iceberg table already exists at creation.
56+
/// Iceberg table does not exist.
5757
TableNotFound,
5858

5959
/// Iceberg feature is not supported.
@@ -278,7 +278,7 @@ impl Error {
278278
/// If you just want to print error with backtrace, use `Debug`, like `format!("{err:?}")`.
279279
///
280280
/// If you use nightly rust, and want to access `iceberg::Error`'s backtrace in the standard way, you can
281-
/// implement a newtype like this:
281+
/// implement a new type like this:
282282
///
283283
/// ```ignore
284284
/// // assume you already have `#![feature(error_generic_member_access)]` on the top of your crate

0 commit comments

Comments
 (0)