Merge pull request JanKaul#223 from JanKaul/more-context-for-unimplemented

JanKaul · JanKaul · commit 9a0f7432a69b · 2025-08-11T12:14:09.000+02:00
provide more context for unimplemented macro
diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml
@@ -13,6 +13,16 @@ jobs:
   test:
     runs-on: ubuntu-latest
     steps:
+    - name: Free disk space
+      uses: jlumbroso/free-disk-space@main
+      with:
+        tool-cache: false
+        android: true
+        dotnet: true
+        haskell: true
+        large-packages: false
+        docker-images: false
+        swap-storage: false
     - uses: actions/checkout@v3
     - name: Build
       run: cargo build --verbose
diff --git a/Makefile b/Makefile
@@ -7,16 +7,16 @@ test-iceberg-rust:
 	cargo test -p iceberg-rust --lib
 
 test-datafusion_iceberg:
-	cargo test -p datafusion_iceberg --tests -j 2 && cargo clean -p datafusion_iceberg
+	cargo test -p datafusion_iceberg --tests -j 2 
 
 test-rest-catalog:
-	cargo test -p iceberg-rest-catalog --lib && cargo clean -p iceberg-rest-catalog
+	cargo test -p iceberg-rest-catalog --lib 
 
 test-file-catalog:
-	cargo test -p iceberg-file-catalog --lib && cargo clean -p iceberg-file-catalog
+	cargo test -p iceberg-file-catalog --lib 
 
 test-sql-catalog:
-	cargo test -p iceberg-sql-catalog --lib && cargo clean -p iceberg-sql-catalog
+	cargo test -p iceberg-sql-catalog --lib 
 clippy:
 	cargo clippy --all-targets --all-features -- -D warnings
 fmt:
diff --git a/catalogs/iceberg-file-catalog/src/lib.rs b/catalogs/iceberg-file-catalog/src/lib.rs
@@ -506,7 +506,7 @@ impl Catalog for FileCatalog {
         _identifier: Identifier,
         _metadata_location: &str,
     ) -> Result<Table, IcebergError> {
-        unimplemented!()
+        unimplemented!("Register table for file catalog")
     }
 }
 
diff --git a/datafusion_iceberg/src/error.rs b/datafusion_iceberg/src/error.rs
@@ -56,7 +56,7 @@ pub enum Error {
     ParseInt(#[from] std::num::ParseIntError),
     /// parse int error
     #[error(transparent)]
-    ConfigBuilder(#[from] crate::table::DataFusionTableConfigBuilderError),
+    DeriveBuilder(#[from] derive_builder::UninitializedFieldError),
 }
 
 impl From<Error> for DataFusionError {
diff --git a/datafusion_iceberg/src/table.rs b/datafusion_iceberg/src/table.rs
@@ -107,6 +107,7 @@ impl From<MaterializedView> for DataFusionTable {
 }
 
 #[derive(Clone, Debug, Builder)]
+#[builder(build_fn(error = "Error"))]
 pub struct DataFusionTableConfig {
     /// With this option, an additional "__data_file_path" column is added to the output of the
     /// TableProvider that contains the path of the data-file the row originates from.
diff --git a/iceberg-rust-spec/src/error.rs b/iceberg-rust-spec/src/error.rs
@@ -55,30 +55,9 @@ pub enum Error {
     /// parse int error
     #[error(transparent)]
     ParseInt(#[from] std::num::ParseIntError),
-    /// table metadata builder
+    /// derive builder
     #[error(transparent)]
-    TableMetadataBuilder(#[from] crate::spec::table_metadata::TableMetadataBuilderError),
-    /// view metadata builder
-    #[error(transparent)]
-    ViewMetadataBuilder(#[from] crate::spec::view_metadata::GeneralViewMetadataBuilderError),
-    /// version builder
-    #[error(transparent)]
-    VersionBuilder(#[from] crate::spec::view_metadata::VersionBuilderError),
-    /// manifest builder
-    #[error(transparent)]
-    ManifestEntryBuilder(#[from] crate::spec::manifest::ManifestEntryBuilderError),
-    /// datafile builder
-    #[error(transparent)]
-    DatafileBuilder(#[from] crate::spec::manifest::DataFileBuilderError),
-    /// snapshot builder
-    #[error(transparent)]
-    SnapshotBuilder(#[from] crate::spec::snapshot::SnapshotBuilderError),
-    /// structype builder
-    #[error(transparent)]
-    StructTypeBuilder(#[from] crate::spec::types::StructTypeBuilderError),
-    /// partition spec builder
-    #[error(transparent)]
-    PartitionSpec(#[from] crate::spec::partition::PartitionSpecBuilderError),
+    DeriveBuilder(#[from] derive_builder::UninitializedFieldError),
 }
 
 impl From<apache_avro::Error> for Error {
diff --git a/iceberg-rust-spec/src/spec/manifest.rs b/iceberg-rust-spec/src/spec/manifest.rs
@@ -34,7 +34,7 @@ use super::{
 /// Entry in manifest with the iceberg spec version 2.
 #[derive(Debug, Serialize, PartialEq, Clone, Getters, Builder)]
 #[serde(into = "ManifestEntryEnum")]
-#[builder(setter(prefix = "with"))]
+#[builder(build_fn(error = "Error"), setter(prefix = "with"))]
 pub struct ManifestEntry {
     /// Table format version
     format_version: FormatVersion,
@@ -516,7 +516,7 @@ impl From<HashMap<i32, Value>> for AvroMap<ByteBuf> {
 }
 
 #[derive(Debug, PartialEq, Clone, Getters, Builder)]
-#[builder(setter(prefix = "with"))]
+#[builder(build_fn(error = "Error"), setter(prefix = "with"))]
 /// DataFile found in Manifest.
 pub struct DataFile {
     ///Type of content in data file.
diff --git a/iceberg-rust-spec/src/spec/materialized_view_metadata.rs b/iceberg-rust-spec/src/spec/materialized_view_metadata.rs
@@ -29,7 +29,7 @@ pub type MaterializedViewMetadata = GeneralViewMetadata<FullIdentifier>;
 pub type MaterializedViewMetadataBuilder = GeneralViewMetadataBuilder<FullIdentifier>;
 
 impl MaterializedViewMetadata {
-    pub fn as_ref(&self) -> TabularMetadataRef {
+    pub fn as_ref(&self) -> TabularMetadataRef<'_> {
         TabularMetadataRef::MaterializedView(self)
     }
 }
diff --git a/iceberg-rust-spec/src/spec/partition.rs b/iceberg-rust-spec/src/spec/partition.rs
@@ -169,7 +169,7 @@ impl PartitionField {
 
 #[derive(Debug, Serialize, Deserialize, PartialEq, Eq, Clone, Default, Builder, Getters)]
 #[serde(rename_all = "kebab-case")]
-#[builder(setter(prefix = "with"))]
+#[builder(build_fn(error = "Error"), setter(prefix = "with"))]
 ///  Partition spec that defines how to produce a tuple of partition values from a record.
 pub struct PartitionSpec {
     /// Identifier for PartitionSpec
diff --git a/iceberg-rust-spec/src/spec/snapshot.rs b/iceberg-rust-spec/src/spec/snapshot.rs
@@ -29,7 +29,7 @@ use _serde::SnapshotEnum;
 
 #[derive(Debug, PartialEq, Eq, Clone, Serialize, Deserialize, Builder, Getters)]
 #[serde(from = "SnapshotEnum", into = "SnapshotEnum")]
-#[builder(setter(prefix = "with"))]
+#[builder(build_fn(error = "Error"), setter(prefix = "with"))]
 /// A snapshot represents the state of a table at some time and is used to access the complete set of data files in the table.
 pub struct Snapshot {
     /// A unique long ID
diff --git a/iceberg-rust-spec/src/spec/table_metadata.rs b/iceberg-rust-spec/src/spec/table_metadata.rs
@@ -204,7 +204,7 @@ impl TableMetadata {
     pub fn current_partition_fields(
         &self,
         branch: Option<&str>,
-    ) -> Result<Vec<BoundPartitionField>, Error> {
+    ) -> Result<Vec<BoundPartitionField<'_>>, Error> {
         let schema = self.current_schema(branch)?;
         let partition_spec = self.default_partition_spec()?;
         partition_fields(partition_spec, schema)
@@ -218,7 +218,10 @@ impl TableMetadata {
     /// # Returns
     /// * `Result<Vec<BoundPartitionField>, Error>` - Vector of partition fields bound to their source schema fields,
     ///   or an error if the schema or partition spec cannot be found
-    pub fn partition_fields(&self, snapshot_id: i64) -> Result<Vec<BoundPartitionField>, Error> {
+    pub fn partition_fields(
+        &self,
+        snapshot_id: i64,
+    ) -> Result<Vec<BoundPartitionField<'_>>, Error> {
         let schema = self.schema(snapshot_id)?;
         self.default_partition_spec()?
             .fields()
@@ -325,7 +328,7 @@ impl TableMetadata {
             .map(|x| *x.sequence_number())
     }
 
-    pub fn as_ref(&self) -> TabularMetadataRef {
+    pub fn as_ref(&self) -> TabularMetadataRef<'_> {
         TabularMetadataRef::Table(self)
     }
 }
@@ -892,6 +895,15 @@ impl From<FormatVersion> for u8 {
     }
 }
 
+impl From<FormatVersion> for i32 {
+    fn from(value: FormatVersion) -> Self {
+        match value {
+            FormatVersion::V1 => 1,
+            FormatVersion::V2 => 2,
+        }
+    }
+}
+
 #[cfg(test)]
 mod tests {
 
diff --git a/iceberg-rust-spec/src/spec/types.rs b/iceberg-rust-spec/src/spec/types.rs
@@ -196,6 +196,7 @@ impl fmt::Display for PrimitiveType {
 /// DataType for a specific struct
 #[derive(Debug, Serialize, PartialEq, Eq, Clone, Builder)]
 #[serde(rename = "struct", tag = "type")]
+#[builder(build_fn(error = "Error"))]
 pub struct StructType {
     /// Struct fields
     #[builder(setter(each(name = "with_struct_field")))]
diff --git a/iceberg-rust-spec/src/spec/values.rs b/iceberg-rust-spec/src/spec/values.rs
@@ -670,7 +670,7 @@ impl Value {
                 precision: 38,
                 scale: dec.scale(),
             }),
-            _ => unimplemented!(),
+            _ => unimplemented!("Datatype for value"),
         }
     }
 
@@ -698,7 +698,7 @@ impl Value {
             Value::String(any) => Box::new(any),
             Value::UUID(any) => Box::new(any),
             Value::Decimal(any) => Box::new(any),
-            _ => unimplemented!(),
+            _ => unimplemented!("Value conversion to any"),
         }
     }
 
diff --git a/iceberg-rust-spec/src/spec/view_metadata.rs b/iceberg-rust-spec/src/spec/view_metadata.rs
@@ -143,7 +143,7 @@ impl<T: Materialization> GeneralViewMetadata<T> {
 }
 
 impl ViewMetadata {
-    pub fn as_ref(&self) -> TabularMetadataRef {
+    pub fn as_ref(&self) -> TabularMetadataRef<'_> {
         TabularMetadataRef::View(self)
     }
 }
@@ -301,6 +301,14 @@ impl From<FormatVersion> for u8 {
     }
 }
 
+impl From<FormatVersion> for i32 {
+    fn from(value: FormatVersion) -> Self {
+        match value {
+            FormatVersion::V1 => 1,
+        }
+    }
+}
+
 #[derive(Debug, Serialize, Deserialize, PartialEq, Eq, Clone, Default, Builder, Getters)]
 #[serde(rename_all = "kebab-case")]
 /// Fields for the version 2 of the view metadata.
diff --git a/iceberg-rust/src/catalog/commit.rs b/iceberg-rust/src/catalog/commit.rs
@@ -420,10 +420,9 @@ pub fn apply_table_updates(
     for update in updates {
         match update {
             TableUpdate::UpgradeFormatVersion { format_version } => {
-                if u8::from(metadata.format_version) == format_version as u8 {
-                    return Ok(());
+                if i32::from(metadata.format_version) != format_version {
+                    unimplemented!("Table format upgrade");
                 }
-                unimplemented!();
             }
             TableUpdate::AssignUuid { uuid } => {
                 metadata.table_uuid = Uuid::parse_str(&uuid)?;
@@ -519,8 +518,10 @@ pub fn apply_view_updates<T: Materialization + 'static>(
 ) -> Result<(), Error> {
     for update in updates {
         match update {
-            ViewUpdate::UpgradeFormatVersion { format_version: _ } => {
-                unimplemented!();
+            ViewUpdate::UpgradeFormatVersion { format_version } => {
+                if i32::from(metadata.format_version.clone()) != format_version {
+                    unimplemented!("Upgrade of format version");
+                }
             }
             ViewUpdate::AssignUuid { uuid } => {
                 metadata.view_uuid = Uuid::parse_str(&uuid)?;
diff --git a/iceberg-rust/src/catalog/create.rs b/iceberg-rust/src/catalog/create.rs
@@ -53,7 +53,7 @@ use super::{identifier::Identifier, Catalog};
 /// can be serialized/deserialized using serde.
 #[derive(Clone, Debug, PartialEq, Serialize, Deserialize, Builder)]
 #[serde(rename_all = "kebab-case")]
-#[builder(build_fn(name = "create"), setter(prefix = "with"))]
+#[builder(build_fn(name = "create", error = "Error"), setter(prefix = "with"))]
 pub struct CreateTable {
     #[builder(setter(into))]
     /// Name of the table
@@ -175,7 +175,7 @@ impl TryInto<TableMetadata> for CreateTable {
 /// can be serialized/deserialized using serde.
 #[derive(Debug, Clone, PartialEq, Serialize, Deserialize, Builder)]
 #[serde(rename_all = "kebab-case")]
-#[builder(build_fn(name = "create"), setter(prefix = "with"))]
+#[builder(build_fn(name = "create", error = "Error"), setter(prefix = "with"))]
 pub struct CreateView<T: Materialization> {
     /// Name of the view
     #[builder(setter(into))]
@@ -285,7 +285,7 @@ impl TryInto<MaterializedViewMetadata> for CreateView<FullIdentifier> {
 /// can be serialized/deserialized using serde.
 #[derive(Debug, Clone, PartialEq, Serialize, Deserialize, Builder)]
 #[serde(rename_all = "kebab-case")]
-#[builder(build_fn(name = "create"), setter(prefix = "with"))]
+#[builder(build_fn(name = "create", error = "Error"), setter(prefix = "with"))]
 pub struct CreateMaterializedView {
     /// Name of the view
     #[builder(setter(into))]
diff --git a/iceberg-rust/src/error.rs b/iceberg-rust/src/error.rs
@@ -89,30 +89,9 @@ pub enum Error {
     /// parse int error
     #[error(transparent)]
     ParseInt(#[from] std::num::ParseIntError),
-    /// table metadata builder
+    /// derive builder
     #[error(transparent)]
-    TableMetadataBuilder(
-        #[from] iceberg_rust_spec::spec::table_metadata::TableMetadataBuilderError,
-    ),
-    /// view metadata builder
-    #[error(transparent)]
-    ViewMetadataBuilder(
-        #[from] iceberg_rust_spec::spec::view_metadata::GeneralViewMetadataBuilderError,
-    ),
-    /// version builder
-    #[error(transparent)]
-    VersionBuilder(#[from] iceberg_rust_spec::spec::view_metadata::VersionBuilderError),
-    /// create table builder
-    #[error(transparent)]
-    CreateTableBuilder(#[from] crate::catalog::create::CreateTableBuilderError),
-    /// create view builder
-    #[error(transparent)]
-    CreateViewBuilder(#[from] crate::catalog::create::CreateViewBuilderError),
-    /// create view builder
-    #[error(transparent)]
-    CreateMaterializedViewBuilder(
-        #[from] crate::catalog::create::CreateMaterializedViewBuilderError,
-    ),
+    DeriveBuilder(#[from] derive_builder::UninitializedFieldError),
 }
 
 impl From<apache_avro::Error> for Error {
diff --git a/iceberg-rust/src/file_format/parquet.rs b/iceberg-rust/src/file_format/parquet.rs
@@ -257,9 +257,7 @@ pub fn parquet_to_datafile(
         builder.with_equality_ids(Some(equality_ids.to_vec()));
     }
 
-    let content = builder
-        .build()
-        .map_err(iceberg_rust_spec::error::Error::from)?;
+    let content = builder.build()?;
     Ok(content)
 }
 
diff --git a/iceberg-rust/src/materialized_view/mod.rs b/iceberg-rust/src/materialized_view/mod.rs
@@ -152,7 +152,7 @@ impl MaterializedView {
     ///
     /// # Returns
     /// A new transaction that can be used to perform multiple operations atomically
-    pub fn new_transaction(&mut self, branch: Option<&str>) -> MaterializedViewTransaction {
+    pub fn new_transaction(&mut self, branch: Option<&str>) -> MaterializedViewTransaction<'_> {
         MaterializedViewTransaction::new(self, branch)
     }
     /// Returns the storage table that contains the materialized data for this view
diff --git a/iceberg-rust/src/object_store/mod.rs b/iceberg-rust/src/object_store/mod.rs
@@ -40,7 +40,7 @@ impl Display for Bucket<'_> {
 
 impl Bucket<'_> {
     /// Get the bucket and coud provider from the location string
-    pub fn from_path(path: &str) -> Result<Bucket, Error> {
+    pub fn from_path(path: &str) -> Result<Bucket<'_>, Error> {
         if path.starts_with("s3://") || path.starts_with("s3a://") {
             let prefix = if path.starts_with("s3://") {
                 "s3://"
diff --git a/iceberg-rust/src/table/manifest.rs b/iceberg-rust/src/table/manifest.rs
@@ -37,7 +37,7 @@ use iceberg_rust_spec::{
 };
 use object_store::ObjectStore;
 
-use crate::{error::Error, spec};
+use crate::error::Error;
 
 type ReaderZip<'a, R> = Zip<AvroReader<'a, R>, Repeat<Arc<(Schema, PartitionSpec, FormatVersion)>>>;
 type ReaderMap<'a, R> = Map<
@@ -130,8 +130,7 @@ impl<R: Read> ManifestReader<'_, R> {
         let partition_spec = PartitionSpec::builder()
             .with_spec_id(spec_id)
             .with_fields(partition_fields)
-            .build()
-            .map_err(spec::error::Error::from)?;
+            .build()?;
         Ok(Self {
             reader: reader
                 .zip(repeat(Arc::new((schema, partition_spec, format_version))))
diff --git a/iceberg-rust/src/table/mod.rs b/iceberg-rust/src/table/mod.rs
@@ -287,7 +287,7 @@ impl Table {
     ///
     /// The transaction must be committed for any changes to take effect.
     /// Multiple operations can be chained within a single transaction.
-    pub fn new_transaction(&mut self, branch: Option<&str>) -> TableTransaction {
+    pub fn new_transaction(&mut self, branch: Option<&str>) -> TableTransaction<'_> {
         TableTransaction::new(self, branch)
     }
 }
diff --git a/iceberg-rust/src/table/transaction/mod.rs b/iceberg-rust/src/table/transaction/mod.rs
@@ -158,10 +158,7 @@ impl<'table> TableTransaction<'table> {
     pub fn append_delete(mut self, files: Vec<DataFile>) -> Self {
         if let Some(ref mut operation) = self.operations[APPEND_INDEX] {
             if let Operation::Append {
-                branch: _,
-                data_files: _,
-                delete_files: old,
-                additional_summary: None,
+                delete_files: old, ..
             } = operation
             {
                 old.extend_from_slice(&files);
diff --git a/iceberg-rust/src/table/transaction/operation.rs b/iceberg-rust/src/table/transaction/operation.rs
diff --git a/iceberg-rust/src/view/mod.rs b/iceberg-rust/src/view/mod.rs

Original file line number	Diff line number	Diff line change
`@@ -506,7 +506,7 @@ impl Catalog for FileCatalog {`
`506`	`506`	`_identifier: Identifier,`
`507`	`507`	`_metadata_location: &str,`
`508`	`508`	`) -> Result<Table, IcebergError> {`
`509`		`- unimplemented!()`
	`509`	`+ unimplemented!("Register table for file catalog")`
`510`	`510`	`}`
`511`	`511`	`}`
`512`	`512`
Original file line number	Diff line number	Diff line change
`@@ -56,7 +56,7 @@ pub enum Error {`
`56`	`56`	`ParseInt(#[from] std::num::ParseIntError),`
`57`	`57`	`/// parse int error`
`58`	`58`	`#[error(transparent)]`
`59`		`- ConfigBuilder(#[from] crate::table::DataFusionTableConfigBuilderError),`
	`59`	`+ DeriveBuilder(#[from] derive_builder::UninitializedFieldError),`
`60`	`60`	`}`
`61`	`61`
`62`	`62`	`impl From<Error> for DataFusionError {`
Original file line number	Diff line number	Diff line change
`@@ -107,6 +107,7 @@ impl From<MaterializedView> for DataFusionTable {`
`107`	`107`	`}`
`108`	`108`
`109`	`109`	`#[derive(Clone, Debug, Builder)]`
	`110`	`+#[builder(build_fn(error = "Error"))]`
`110`	`111`	`pub struct DataFusionTableConfig {`
`111`	`112`	`/// With this option, an additional "__data_file_path" column is added to the output of the`
`112`	`113`	`/// TableProvider that contains the path of the data-file the row originates from.`
Original file line number	Diff line number	Diff line change
`@@ -29,7 +29,7 @@ pub type MaterializedViewMetadata = GeneralViewMetadata<FullIdentifier>;`
`29`	`29`	`pub type MaterializedViewMetadataBuilder = GeneralViewMetadataBuilder<FullIdentifier>;`
`30`	`30`
`31`	`31`	`impl MaterializedViewMetadata {`
`32`		`- pub fn as_ref(&self) -> TabularMetadataRef {`
	`32`	`+ pub fn as_ref(&self) -> TabularMetadataRef<'_> {`
`33`	`33`	`TabularMetadataRef::MaterializedView(self)`
`34`	`34`	`}`
`35`	`35`	`}`