Initial schema compat

Add schema compatibility tests

Add error-based schema compatibility check

Remove old schema compat

change naming, remove redundant test

Improve comments, add check that field names aren't case-insensitive duplicates

Change ordering, remove unnecessary comment

Add comments to can_read_as methods

Add schema compatibility check to CDF, update cdf tests

remove todo comments

Add clarifying comments

Author: OussamaSaoudi-db

kernel/src/table_changes/log_replay.rs

@@ -15,6 +15,7 @@ use crate::expressions::{column_name, ColumnName};
 use crate::path::ParsedLogPath;
 use crate::scan::data_skipping::DataSkippingFilter;
 use crate::scan::state::DvInfo;
+use crate::schema::compare::SchemaComparison;
 use crate::schema::{ArrayType, ColumnNamesAndTypes, DataType, MapType, SchemaRef, StructType};
 use crate::table_changes::scan_file::{cdf_scan_row_expression, cdf_scan_row_schema};
 use crate::table_changes::{check_cdf_table_properties, ensure_cdf_read_supported};
@@ -79,9 +80,8 @@ pub(crate) fn table_changes_action_iter(
 ///       phase, so we must perform it ahead of time in phase 1.
 ///     - Ensure that reading is supported on any protocol updates.
 ///     - Ensure that Change Data Feed is enabled for any metadata update. See  [`TableProperties`]
-///     - Ensure that any schema update is compatible with the provided `schema`. Currently, schema
-///       compatibility is checked through schema equality. This will be expanded in the future to
-///       allow limited schema evolution.
+///     - Ensure that any schema update is compatible with the provided `schema`. To see how schema
+///       compatibility is defined, see [`StructType::can_read_as`].
 ///
 /// Note: We check the protocol, change data feed enablement, and schema compatibility in phase 1
 /// in order to detect errors and fail early.
@@ -181,11 +181,8 @@ impl LogReplayScanner {
             }
             if let Some((schema, configuration)) = visitor.metadata_info {
                 let schema: StructType = serde_json::from_str(&schema)?;
-                // Currently, schema compatibility is defined as having equal schema types. In the
-                // future, more permisive schema evolution will be supported.
-                // See: https://github.com/delta-io/delta-kernel-rs/issues/523
                 require!(
-                    table_schema.as_ref() == &schema,
+                    schema.can_read_as(table_schema).is_ok(),
                     Error::change_data_feed_incompatible_schema(table_schema, &schema)
                 );
                 let table_properties = TableProperties::from(configuration);

kernel/src/table_changes/log_replay/tests.rs

@@ -185,7 +185,7 @@ async fn column_mapping_should_fail() {
 // Note: This should be removed once type widening support is added for CDF
 #[tokio::test]
 async fn incompatible_schemas_fail() {
-    async fn assert_incompatible_schema(commit_schema: StructType, cdf_schema: StructType) {
+    async fn assert_schema_check(commit_schema: StructType, cdf_schema: StructType, passes: bool) {
         let engine = Arc::new(SyncEngine::new());
         let mut mock_table = LocalMockTable::new();
 
@@ -210,29 +210,38 @@ async fn incompatible_schemas_fail() {
                 .unwrap()
                 .try_collect();
 
-        assert!(matches!(
-            res,
-            Err(Error::ChangeDataFeedIncompatibleSchema(_, _))
-        ));
+        if passes {
+            assert!(res.is_ok());
+        } else {
+            assert!(matches!(
+                res,
+                Err(Error::ChangeDataFeedIncompatibleSchema(_, _))
+            ));
+        }
     }
 
+    // Column `year` exists in commit schema, but not in the read schema.
+    //
     // The CDF schema has fields: `id: int` and `value: string`.
-    // This commit has schema with fields: `id: long`, `value: string` and `year: int` (nullable).
+    // This commit has schema with fields: `id: int`, `value: string` and `year: int` (nullable).
     let schema = StructType::new([
         StructField::nullable("id", DataType::LONG),
         StructField::nullable("value", DataType::STRING),
         StructField::nullable("year", DataType::INTEGER),
     ]);
-    assert_incompatible_schema(schema, get_schema()).await;
+    assert_schema_check(schema, get_schema(), false).await;
 
+    // Commit schema's `id` column has wider type than in the read schema.
+    //
     // The CDF schema has fields: `id: int` and `value: string`.
     // This commit has schema with fields: `id: long` and `value: string`.
     let schema = StructType::new([
         StructField::nullable("id", DataType::LONG),
         StructField::nullable("value", DataType::STRING),
     ]);
-    assert_incompatible_schema(schema, get_schema()).await;
+    assert_schema_check(schema, get_schema(), false).await;
 
+    // Commit schema's `id` column is narrower than in the read schema.
     // NOTE: Once type widening is supported, this should not return an error.
     //
     // The CDF schema has fields: `id: long` and `value: string`.
@@ -245,31 +254,34 @@ async fn incompatible_schemas_fail() {
         StructField::nullable("id", DataType::INTEGER),
         StructField::nullable("value", DataType::STRING),
     ]);
-    assert_incompatible_schema(cdf_schema, commit_schema).await;
+    assert_schema_check(commit_schema, cdf_schema, false).await;
 
-    // Note: Once schema evolution is supported, this should not return an error.
+    // Commit schema's `id` column has an incompatible type with the read schema's.
     //
-    // The CDF schema has fields: nullable `id`  and nullable `value`.
-    // This commit has schema with fields: non-nullable `id` and nullable `value`.
+    // The CDF schema has fields: `id: int` and `value: string`.
+    // This commit has schema with fields: `id: string` and `value: string`.
     let schema = StructType::new([
-        StructField::not_null("id", DataType::LONG),
+        StructField::nullable("id", DataType::STRING),
         StructField::nullable("value", DataType::STRING),
     ]);
-    assert_incompatible_schema(schema, get_schema()).await;
+    assert_schema_check(schema, get_schema(), false).await;
 
-    // The CDF schema has fields: `id: int` and `value: string`.
-    // This commit has schema with fields:`id: string` and `value: string`.
+    // Commit schema's `id` column is non-nullable, but is nullable in the read schema.
+    //
+    // The CDF schema has fields: nullable `id`  and nullable `value`.
+    // This commit has schema with fields: non-nullable `id` and nullable `value`.
     let schema = StructType::new([
-        StructField::nullable("id", DataType::STRING),
-        StructField::nullable("value", DataType::STRING),
+        StructField::new("id", DataType::INTEGER, false),
+        StructField::new("value", DataType::STRING, true),
     ]);
-    assert_incompatible_schema(schema, get_schema()).await;
+    assert_schema_check(schema, get_schema(), true).await;
 
-    // Note: Once schema evolution is supported, this should not return an error.
+    // Commit schema is missing a nullable `value` column that's found in the read schema.
+    //
     // The CDF schema has fields: `id` (nullable) and `value` (nullable).
     // This commit has schema with fields: `id` (nullable).
     let schema = get_schema().project_as_struct(&["id"]).unwrap();
-    assert_incompatible_schema(schema, get_schema()).await;
+    assert_schema_check(schema, get_schema(), true).await;
 }
 
 #[tokio::test]

kernel/src/table_changes/mod.rs

@@ -40,6 +40,7 @@ use url::Url;
 use crate::actions::{ensure_supported_features, Protocol};
 use crate::log_segment::LogSegment;
 use crate::path::AsUrl;
+use crate::schema::compare::SchemaComparison;
 use crate::schema::{DataType, Schema, StructField, StructType};
 use crate::snapshot::Snapshot;
 use crate::table_features::{ColumnMappingMode, ReaderFeatures};
@@ -173,11 +174,9 @@ impl TableChanges {
 
         // Verify that the start and end schemas are compatible. We must still check schema
         // compatibility for each schema update in the CDF range.
-        // Note: Schema compatibility check will be changed in the future to be more flexible.
-        // See issue [#523](https://github.com/delta-io/delta-kernel-rs/issues/523)
-        if start_snapshot.schema() != end_snapshot.schema() {
+        if let Err(err) = start_snapshot.schema().can_read_as(end_snapshot.schema()) {
             return Err(Error::generic(format!(
-                "Failed to build TableChanges: Start and end version schemas are different. Found start version schema {:?} and end version schema {:?}", start_snapshot.schema(), end_snapshot.schema(),
+                "Failed to build TableChanges: {}\n Found start version schema {:?} and end version schema {:?}", err, start_snapshot.schema(), end_snapshot.schema(),
             )));
         }
 
@@ -303,7 +302,7 @@ mod tests {
         let path = "./tests/data/table-with-cdf";
         let engine = Box::new(SyncEngine::new());
         let table = Table::try_from_uri(path).unwrap();
-        let expected_msg = "Failed to build TableChanges: Start and end version schemas are different. Found start version schema StructType { type_name: \"struct\", fields: {\"part\": StructField { name: \"part\", data_type: Primitive(Integer), nullable: true, metadata: {} }, \"id\": StructField { name: \"id\", data_type: Primitive(Integer), nullable: true, metadata: {} }} } and end version schema StructType { type_name: \"struct\", fields: {\"part\": StructField { name: \"part\", data_type: Primitive(Integer), nullable: true, metadata: {} }, \"id\": StructField { name: \"id\", data_type: Primitive(Integer), nullable: false, metadata: {} }} }";
+        let expected_msg = "Failed to build TableChanges: Generic delta kernel error: Read field is non-nullable while this field is nullable\n Found start version schema StructType { type_name: \"struct\", fields: {\"part\": StructField { name: \"part\", data_type: Primitive(Integer), nullable: true, metadata: {} }, \"id\": StructField { name: \"id\", data_type: Primitive(Integer), nullable: true, metadata: {} }} } and end version schema StructType { type_name: \"struct\", fields: {\"part\": StructField { name: \"part\", data_type: Primitive(Integer), nullable: true, metadata: {} }, \"id\": StructField { name: \"id\", data_type: Primitive(Integer), nullable: false, metadata: {} }} }";
 
         // A field in the schema goes from being nullable to non-nullable
         let table_changes_res = table.table_changes(engine.as_ref(), 3, 4);