Made snapshot.schema() return a SchemaRef

maruschin · maruschin · commit 485849bc4bc7 · 2025-03-21T12:33:03.000+10:00
diff --git a/ffi/src/lib.rs b/ffi/src/lib.rs
@@ -619,7 +619,7 @@ pub unsafe extern "C" fn version(snapshot: Handle<SharedSnapshot>) -> u64 {
 #[no_mangle]
 pub unsafe extern "C" fn logical_schema(snapshot: Handle<SharedSnapshot>) -> Handle<SharedSchema> {
     let snapshot = unsafe { snapshot.as_ref() };
-    Arc::new(snapshot.schema().clone()).into()
+    snapshot.schema().into()
 }
 
 /// Free a schema
diff --git a/kernel/src/scan/mod.rs b/kernel/src/scan/mod.rs
@@ -96,9 +96,7 @@ impl ScanBuilder {
     /// perform actual data reads.
     pub fn build(self) -> DeltaResult<Scan> {
         // if no schema is provided, use snapshot's entire schema (e.g. SELECT *)
-        let logical_schema = self
-            .schema
-            .unwrap_or_else(|| self.snapshot.schema().clone().into());
+        let logical_schema = self.schema.unwrap_or_else(|| self.snapshot.schema());
         let state_info = get_state_info(
             logical_schema.as_ref(),
             &self.snapshot.metadata().partition_columns,
diff --git a/kernel/src/schema/mod.rs b/kernel/src/schema/mod.rs
@@ -331,9 +331,9 @@ impl InvariantChecker {
     ///
     /// This traverses the entire schema to check for the presence of the "delta.invariants"
     /// metadata key.
-    pub(crate) fn has_invariants(schema: &Schema) -> bool {
+    pub(crate) fn has_invariants(schema: SchemaRef) -> bool {
         let mut checker = InvariantChecker::default();
-        let _ = checker.transform_struct(schema);
+        let _ = checker.transform_struct(schema.as_ref());
         checker.has_invariants
     }
 }
@@ -1251,11 +1251,11 @@ mod tests {
     #[test]
     fn test_has_invariants() {
         // Schema with no invariants
-        let schema = StructType::new([
+        let schema = Arc::new(StructType::new([
             StructField::nullable("a", DataType::STRING),
             StructField::nullable("b", DataType::INTEGER),
-        ]);
-        assert!(!InvariantChecker::has_invariants(&schema));
+        ]));
+        assert!(!InvariantChecker::has_invariants(schema));
 
         // Schema with top-level invariant
         let mut field = StructField::nullable("c", DataType::STRING);
@@ -1264,8 +1264,11 @@ mod tests {
             MetadataValue::String("c > 0".to_string()),
         );
 
-        let schema = StructType::new([StructField::nullable("a", DataType::STRING), field]);
-        assert!(InvariantChecker::has_invariants(&schema));
+        let schema = Arc::new(StructType::new([
+            StructField::nullable("a", DataType::STRING),
+            field,
+        ]));
+        assert!(InvariantChecker::has_invariants(schema));
 
         // Schema with nested invariant in a struct
         let nested_field = StructField::nullable(
@@ -1280,12 +1283,12 @@ mod tests {
             }]),
         );
 
-        let schema = StructType::new([
+        let schema = Arc::new(StructType::new([
             StructField::nullable("a", DataType::STRING),
             StructField::nullable("b", DataType::INTEGER),
             nested_field,
-        ]);
-        assert!(InvariantChecker::has_invariants(&schema));
+        ]));
+        assert!(InvariantChecker::has_invariants(schema));
 
         // Schema with nested invariant in an array of structs
         let array_field = StructField::nullable(
@@ -1303,12 +1306,12 @@ mod tests {
             ),
         );
 
-        let schema = StructType::new([
+        let schema = Arc::new(StructType::new([
             StructField::nullable("a", DataType::STRING),
             StructField::nullable("b", DataType::INTEGER),
             array_field,
-        ]);
-        assert!(InvariantChecker::has_invariants(&schema));
+        ]));
+        assert!(InvariantChecker::has_invariants(schema));
 
         // Schema with nested invariant in a map value that's a struct
         let map_field = StructField::nullable(
@@ -1327,11 +1330,11 @@ mod tests {
             ),
         );
 
-        let schema = StructType::new([
+        let schema = Arc::new(StructType::new([
             StructField::nullable("a", DataType::STRING),
             StructField::nullable("b", DataType::INTEGER),
             map_field,
-        ]);
-        assert!(InvariantChecker::has_invariants(&schema));
+        ]));
+        assert!(InvariantChecker::has_invariants(schema));
     }
 }
diff --git a/kernel/src/snapshot.rs b/kernel/src/snapshot.rs
@@ -9,7 +9,7 @@ use url::Url;
 use crate::actions::{Metadata, Protocol};
 use crate::log_segment::LogSegment;
 use crate::scan::ScanBuilder;
-use crate::schema::Schema;
+use crate::schema::{Schema, SchemaRef};
 use crate::table_configuration::TableConfiguration;
 use crate::table_features::ColumnMappingMode;
 use crate::table_properties::TableProperties;
@@ -98,8 +98,7 @@ impl Snapshot {
     }
 
     /// Table [`Schema`] at this `Snapshot`s version.
-    // TODO should this return SchemaRef?
-    pub fn schema(&self) -> &Schema {
+    pub fn schema(&self) -> SchemaRef {
         self.table_configuration.schema()
     }
 
@@ -203,7 +202,6 @@ mod tests {
     use crate::engine::default::filesystem::ObjectStoreFileSystemClient;
     use crate::engine::sync::SyncEngine;
     use crate::path::ParsedLogPath;
-    use crate::schema::StructType;
 
     #[test]
     fn test_snapshot_read_metadata() {
@@ -219,8 +217,8 @@ mod tests {
         assert_eq!(snapshot.protocol(), &expected);
 
         let schema_string = r#"{"type":"struct","fields":[{"name":"value","type":"integer","nullable":true,"metadata":{}}]}"#;
-        let expected: StructType = serde_json::from_str(schema_string).unwrap();
-        assert_eq!(snapshot.schema(), &expected);
+        let expected: SchemaRef = serde_json::from_str(schema_string).unwrap();
+        assert_eq!(snapshot.schema(), expected);
     }
 
     #[test]
@@ -237,8 +235,8 @@ mod tests {
         assert_eq!(snapshot.protocol(), &expected);
 
         let schema_string = r#"{"type":"struct","fields":[{"name":"value","type":"integer","nullable":true,"metadata":{}}]}"#;
-        let expected: StructType = serde_json::from_str(schema_string).unwrap();
-        assert_eq!(snapshot.schema(), &expected);
+        let expected: SchemaRef = serde_json::from_str(schema_string).unwrap();
+        assert_eq!(snapshot.schema(), expected);
     }
 
     #[test]
diff --git a/kernel/src/table_changes/scan.rs b/kernel/src/table_changes/scan.rs
@@ -197,7 +197,7 @@ impl TableChangesScan {
             PhysicalPredicate::Some(predicate, schema) => Some((predicate, schema)),
             PhysicalPredicate::None => None,
         };
-        let schema = self.table_changes.end_snapshot.schema().clone().into();
+        let schema = self.table_changes.end_snapshot.schema();
         let it = table_changes_action_iter(engine, commits, schema, physical_predicate)?;
         Ok(Some(it).into_iter().flatten())
     }
diff --git a/kernel/src/table_configuration.rs b/kernel/src/table_configuration.rs
@@ -14,7 +14,7 @@ use std::sync::{Arc, LazyLock};
 use url::Url;
 
 use crate::actions::{ensure_supported_features, Metadata, Protocol};
-use crate::schema::{InvariantChecker, Schema, SchemaRef};
+use crate::schema::{InvariantChecker, SchemaRef};
 use crate::table_features::{
     column_mapping_mode, validate_schema_column_mapping, ColumnMappingMode, ReaderFeatures,
     WriterFeatures,
@@ -101,10 +101,10 @@ impl TableConfiguration {
         &self.protocol
     }
 
-    /// The [`Schema`] of for this table at this version.
+    /// The logical schema ([`SchemaRef`]) of this table at this version.
     #[cfg_attr(feature = "developer-visibility", visibility::make(pub))]
-    pub(crate) fn schema(&self) -> &Schema {
-        self.schema.as_ref()
+    pub(crate) fn schema(&self) -> SchemaRef {
+        self.schema.clone()
     }
 
     /// The [`TableProperties`] of this table at this version.
diff --git a/kernel/src/transaction.rs b/kernel/src/transaction.rs
@@ -151,8 +151,9 @@ impl Transaction {
         // for now, we just pass through all the columns except partition columns.
         // note this is _incorrect_ if table config deems we need partition columns.
         let partition_columns = &self.read_snapshot.metadata().partition_columns;
-        let fields = self.read_snapshot.schema().fields();
-        let fields = fields
+        let schema = self.read_snapshot.schema();
+        let fields = schema
+            .fields()
             .filter(|f| !partition_columns.contains(f.name()))
             .map(|f| Expression::column([f.name()]));
         Expression::struct_from(fields)
@@ -167,11 +168,7 @@ impl Transaction {
         let target_dir = self.read_snapshot.table_root();
         let snapshot_schema = self.read_snapshot.schema();
         let logical_to_physical = self.generate_logical_to_physical();
-        WriteContext::new(
-            target_dir.clone(),
-            Arc::new(snapshot_schema.clone()),
-            logical_to_physical,
-        )
+        WriteContext::new(target_dir.clone(), snapshot_schema, logical_to_physical)
     }
 
     /// Add write metadata about files to include in the transaction. This API can be called

Original file line number	Diff line number	Diff line change
`@@ -619,7 +619,7 @@ pub unsafe extern "C" fn version(snapshot: Handle<SharedSnapshot>) -> u64 {`
`619`	`619`	`#[no_mangle]`
`620`	`620`	`pub unsafe extern "C" fn logical_schema(snapshot: Handle<SharedSnapshot>) -> Handle<SharedSchema> {`
`621`	`621`	`let snapshot = unsafe { snapshot.as_ref() };`
`622`		`- Arc::new(snapshot.schema().clone()).into()`
	`622`	`+ snapshot.schema().into()`
`623`	`623`	`}`
`624`	`624`
`625`	`625`	`/// Free a schema`
Original file line number	Diff line number	Diff line change
`@@ -197,7 +197,7 @@ impl TableChangesScan {`
`197`	`197`	`PhysicalPredicate::Some(predicate, schema) => Some((predicate, schema)),`
`198`	`198`	`PhysicalPredicate::None => None,`
`199`	`199`	`};`
`200`		`- let schema = self.table_changes.end_snapshot.schema().clone().into();`
	`200`	`+ let schema = self.table_changes.end_snapshot.schema();`
`201`	`201`	`let it = table_changes_action_iter(engine, commits, schema, physical_predicate)?;`
`202`	`202`	`Ok(Some(it).into_iter().flatten())`
`203`	`203`	`}`