From 025d68d2ebaeee04ae91060c52dc7fa03ff630cf Mon Sep 17 00:00:00 2001 From: DrakeLin Date: Mon, 29 Sep 2025 23:48:54 +0000 Subject: [PATCH 1/6] enablement --- kernel/src/table_configuration.rs | 120 ++++++++++++++++++++++-------- 1 file changed, 90 insertions(+), 30 deletions(-) diff --git a/kernel/src/table_configuration.rs b/kernel/src/table_configuration.rs index 143497fb3..cb118934f 100644 --- a/kernel/src/table_configuration.rs +++ b/kernel/src/table_configuration.rs @@ -23,6 +23,19 @@ use crate::table_properties::TableProperties; use crate::{DeltaResult, Error, Version}; use delta_kernel_derive::internal_api; +/// Information about in-commit timestamp enablement state. +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) enum InCommitTimestampInfo { + /// In-commit timestamps is not enabled + NotEnabled, + /// In-commit timestamps is enabled + Enabled { + /// Enablement information, if available. `None` indicates the table was created + /// with ICT enabled from the beginning (no enablement properties needed). + enablement: Option<(Version, i64)> + }, +} + /// Holds all the configuration for a table at a specific version. This includes the supported /// reader and writer features, table properties, schema, version, and table root. This can be used /// to check whether a table supports a feature or has it enabled. For example, deletion vector @@ -317,17 +330,20 @@ impl TableConfiguration { .unwrap_or(false) } - /// If in-commit timestamps is enabled, returns a tuple of the in-commit timestamp enablement - /// version and timestamp. + + /// Returns information about in-commit timestamp enablement state. /// - /// If in-commit timestamps is not supported, or not enabled, this returns `None`. - /// If in-commit timestams is enabled, but the enablement version or timestamp is not present, - /// this returns an error. + /// This function assumes in-commit timestamps is supported. If it's not supported, + /// that should be checked at a higher level and treated as an error. + /// + /// Returns an error if only one of the enablement properties is present, as this indicates + /// an inconsistent state. #[allow(unused)] - pub(crate) fn in_commit_timestamp_enablement(&self) -> DeltaResult> { + pub(crate) fn in_commit_timestamp_info(&self) -> DeltaResult { if !self.is_in_commit_timestamps_enabled() { - return Ok(None); + return Ok(InCommitTimestampInfo::NotEnabled); } + let enablement_version = self .table_properties() .in_commit_timestamp_enablement_version; @@ -335,18 +351,19 @@ impl TableConfiguration { .table_properties() .in_commit_timestamp_enablement_timestamp; - let ict_error = |err: &str| { - Error::generic(format!( - "In-commit timestamp enabled, but missing Enablement version or timestamp. {err}" - )) - }; match (enablement_version, enablement_timestamp) { - (Some(version), Some(timestamp)) => Ok(Some((version, timestamp))), - (Some(_), None) => Err(ict_error("Enablement timestamp is not present")), - (None, Some(_)) => Err(ict_error("Enablement version is not present")), - (None, None) => Err(ict_error( - "Enablement version and timestamp are not present.", + (Some(version), Some(timestamp)) => Ok(InCommitTimestampInfo::Enabled { + enablement: Some((version, timestamp)) + }), + (Some(_), None) => Err(Error::generic( + "In-commit timestamp enabled, but enablement timestamp is missing while enablement version is present" + )), + (None, Some(_)) => Err(Error::generic( + "In-commit timestamp enabled, but enablement version is missing while enablement timestamp is present" )), + (None, None) => Ok(InCommitTimestampInfo::Enabled { + enablement: None + }), // Normal for tables where ICT was enabled from the beginning } } @@ -423,7 +440,7 @@ mod test { use crate::utils::test_utils::assert_result_error_with_message; use crate::Error; - use super::TableConfiguration; + use super::{TableConfiguration, InCommitTimestampInfo}; #[test] fn dv_supported_not_enabled() { @@ -515,11 +532,29 @@ mod test { let table_config = TableConfiguration::try_new(metadata, protocol, table_root, 0).unwrap(); assert!(table_config.is_in_commit_timestamps_supported()); assert!(table_config.is_in_commit_timestamps_enabled()); - let enablement = table_config.in_commit_timestamp_enablement().unwrap(); - assert_eq!(enablement, Some((5, 100))) + let info = table_config.in_commit_timestamp_info().unwrap(); + assert_eq!(info, InCommitTimestampInfo::Enabled { enablement: Some((5, 100)) }) + } + #[test] + fn ict_supported_and_not_enabled() { + let schema = StructType::new_unchecked([StructField::nullable("value", DataType::INTEGER)]); + let metadata = Metadata::try_new(None, None, schema, vec![], 0, HashMap::new()).unwrap(); + let protocol = Protocol::try_new( + 3, + 7, + Some::>(vec![]), + Some([WriterFeature::InCommitTimestamp]), + ) + .unwrap(); + let table_root = Url::try_from("file:///").unwrap(); + let table_config = TableConfiguration::try_new(metadata, protocol, table_root, 0).unwrap(); + assert!(table_config.is_in_commit_timestamps_supported()); + assert!(!table_config.is_in_commit_timestamps_enabled()); + let info = table_config.in_commit_timestamp_info().unwrap(); + assert_eq!(info, InCommitTimestampInfo::NotEnabled); } #[test] - fn ict_supported_and_enabled_without_enablement_info() { + fn ict_enabled_with_partial_enablement_info() { let schema = StructType::new_unchecked([StructField::nullable("value", DataType::INTEGER)]); let metadata = Metadata::try_new( None, @@ -527,10 +562,17 @@ mod test { schema, vec![], 0, - HashMap::from_iter([( - "delta.enableInCommitTimestamps".to_string(), - "true".to_string(), - )]), + HashMap::from_iter([ + ( + "delta.enableInCommitTimestamps".to_string(), + "true".to_string(), + ), + ( + "delta.inCommitTimestampEnablementVersion".to_string(), + "5".to_string(), + ), + // Missing enablement timestamp + ]), ) .unwrap(); let protocol = Protocol::try_new( @@ -544,14 +586,28 @@ mod test { let table_config = TableConfiguration::try_new(metadata, protocol, table_root, 0).unwrap(); assert!(table_config.is_in_commit_timestamps_supported()); assert!(table_config.is_in_commit_timestamps_enabled()); + // When ICT is enabled but only one enablement property is present, this is an inconsistent state + // and should return an error assert!(matches!( - table_config.in_commit_timestamp_enablement(), - Err(Error::Generic(msg)) if msg.contains("Enablement version and timestamp are not present."))); + table_config.in_commit_timestamp_info(), + Err(Error::Generic(msg)) if msg.contains("enablement timestamp is missing while enablement version is present") + )); } #[test] - fn ict_supported_and_not_enabled() { + fn ict_enabled_from_table_creation() { let schema = StructType::new_unchecked([StructField::nullable("value", DataType::INTEGER)]); - let metadata = Metadata::try_new(None, None, schema, vec![], 0, HashMap::new()).unwrap(); + let metadata = Metadata::try_new( + None, + None, + schema, + vec![], + 0, // Table creation version + HashMap::from_iter([( + "delta.enableInCommitTimestamps".to_string(), + "true".to_string(), + )]), + ) + .unwrap(); let protocol = Protocol::try_new( 3, 7, @@ -562,7 +618,11 @@ mod test { let table_root = Url::try_from("file:///").unwrap(); let table_config = TableConfiguration::try_new(metadata, protocol, table_root, 0).unwrap(); assert!(table_config.is_in_commit_timestamps_supported()); - assert!(!table_config.is_in_commit_timestamps_enabled()); + assert!(table_config.is_in_commit_timestamps_enabled()); + // When ICT is enabled from table creation (version 0), it's perfectly normal + // for enablement properties to be missing + let info = table_config.in_commit_timestamp_info().unwrap(); + assert_eq!(info, InCommitTimestampInfo::Enabled { enablement: None }); } #[test] fn fails_on_unsupported_feature() { From 85c406e6e2bdc78bf820b60d5e3593dce9b34ce7 Mon Sep 17 00:00:00 2001 From: DrakeLin Date: Mon, 29 Sep 2025 23:57:18 +0000 Subject: [PATCH 2/6] fix --- kernel/src/table_configuration.rs | 91 +++++++++++++++---------------- 1 file changed, 43 insertions(+), 48 deletions(-) diff --git a/kernel/src/table_configuration.rs b/kernel/src/table_configuration.rs index cb118934f..6830edf56 100644 --- a/kernel/src/table_configuration.rs +++ b/kernel/src/table_configuration.rs @@ -25,7 +25,7 @@ use delta_kernel_derive::internal_api; /// Information about in-commit timestamp enablement state. #[derive(Debug, Clone, PartialEq, Eq)] -pub(crate) enum InCommitTimestampInfo { +pub(crate) enum InCommitTimestampEnablement { /// In-commit timestamps is not enabled NotEnabled, /// In-commit timestamps is enabled @@ -333,15 +333,12 @@ impl TableConfiguration { /// Returns information about in-commit timestamp enablement state. /// - /// This function assumes in-commit timestamps is supported. If it's not supported, - /// that should be checked at a higher level and treated as an error. - /// /// Returns an error if only one of the enablement properties is present, as this indicates /// an inconsistent state. #[allow(unused)] - pub(crate) fn in_commit_timestamp_info(&self) -> DeltaResult { + pub(crate) fn in_commit_timestamp_enablement(&self) -> DeltaResult { if !self.is_in_commit_timestamps_enabled() { - return Ok(InCommitTimestampInfo::NotEnabled); + return Ok(InCommitTimestampEnablement::NotEnabled); } let enablement_version = self @@ -352,7 +349,7 @@ impl TableConfiguration { .in_commit_timestamp_enablement_timestamp; match (enablement_version, enablement_timestamp) { - (Some(version), Some(timestamp)) => Ok(InCommitTimestampInfo::Enabled { + (Some(version), Some(timestamp)) => Ok(InCommitTimestampEnablement::Enabled { enablement: Some((version, timestamp)) }), (Some(_), None) => Err(Error::generic( @@ -361,9 +358,9 @@ impl TableConfiguration { (None, Some(_)) => Err(Error::generic( "In-commit timestamp enabled, but enablement version is missing while enablement timestamp is present" )), - (None, None) => Ok(InCommitTimestampInfo::Enabled { + (None, None) => Ok(InCommitTimestampEnablement::Enabled { enablement: None - }), // Normal for tables where ICT was enabled from the beginning + }), } } @@ -440,7 +437,7 @@ mod test { use crate::utils::test_utils::assert_result_error_with_message; use crate::Error; - use super::{TableConfiguration, InCommitTimestampInfo}; + use super::{TableConfiguration, InCommitTimestampEnablement}; #[test] fn dv_supported_not_enabled() { @@ -497,6 +494,37 @@ mod test { assert!(table_config.is_deletion_vector_enabled()); } #[test] + fn ict_enabled_from_table_creation() { + let schema = StructType::new_unchecked([StructField::nullable("value", DataType::INTEGER)]); + let metadata = Metadata::try_new( + None, + None, + schema, + vec![], + 0, // Table creation version + HashMap::from_iter([( + "delta.enableInCommitTimestamps".to_string(), + "true".to_string(), + )]), + ) + .unwrap(); + let protocol = Protocol::try_new( + 3, + 7, + Some::>(vec![]), + Some([WriterFeature::InCommitTimestamp]), + ) + .unwrap(); + let table_root = Url::try_from("file:///").unwrap(); + let table_config = TableConfiguration::try_new(metadata, protocol, table_root, 0).unwrap(); + assert!(table_config.is_in_commit_timestamps_supported()); + assert!(table_config.is_in_commit_timestamps_enabled()); + // When ICT is enabled from table creation (version 0), it's perfectly normal + // for enablement properties to be missing + let info = table_config.in_commit_timestamp_enablement().unwrap(); + assert_eq!(info, InCommitTimestampEnablement::Enabled { enablement: None }); + } + #[test] fn ict_supported_and_enabled() { let schema = StructType::new_unchecked([StructField::nullable("value", DataType::INTEGER)]); let metadata = Metadata::try_new( @@ -532,8 +560,8 @@ mod test { let table_config = TableConfiguration::try_new(metadata, protocol, table_root, 0).unwrap(); assert!(table_config.is_in_commit_timestamps_supported()); assert!(table_config.is_in_commit_timestamps_enabled()); - let info = table_config.in_commit_timestamp_info().unwrap(); - assert_eq!(info, InCommitTimestampInfo::Enabled { enablement: Some((5, 100)) }) + let info = table_config.in_commit_timestamp_enablement().unwrap(); + assert_eq!(info, InCommitTimestampEnablement::Enabled { enablement: Some((5, 100)) }) } #[test] fn ict_supported_and_not_enabled() { @@ -550,8 +578,8 @@ mod test { let table_config = TableConfiguration::try_new(metadata, protocol, table_root, 0).unwrap(); assert!(table_config.is_in_commit_timestamps_supported()); assert!(!table_config.is_in_commit_timestamps_enabled()); - let info = table_config.in_commit_timestamp_info().unwrap(); - assert_eq!(info, InCommitTimestampInfo::NotEnabled); + let info = table_config.in_commit_timestamp_enablement().unwrap(); + assert_eq!(info, InCommitTimestampEnablement::NotEnabled); } #[test] fn ict_enabled_with_partial_enablement_info() { @@ -586,45 +614,12 @@ mod test { let table_config = TableConfiguration::try_new(metadata, protocol, table_root, 0).unwrap(); assert!(table_config.is_in_commit_timestamps_supported()); assert!(table_config.is_in_commit_timestamps_enabled()); - // When ICT is enabled but only one enablement property is present, this is an inconsistent state - // and should return an error assert!(matches!( - table_config.in_commit_timestamp_info(), + table_config.in_commit_timestamp_enablement(), Err(Error::Generic(msg)) if msg.contains("enablement timestamp is missing while enablement version is present") )); } #[test] - fn ict_enabled_from_table_creation() { - let schema = StructType::new_unchecked([StructField::nullable("value", DataType::INTEGER)]); - let metadata = Metadata::try_new( - None, - None, - schema, - vec![], - 0, // Table creation version - HashMap::from_iter([( - "delta.enableInCommitTimestamps".to_string(), - "true".to_string(), - )]), - ) - .unwrap(); - let protocol = Protocol::try_new( - 3, - 7, - Some::>(vec![]), - Some([WriterFeature::InCommitTimestamp]), - ) - .unwrap(); - let table_root = Url::try_from("file:///").unwrap(); - let table_config = TableConfiguration::try_new(metadata, protocol, table_root, 0).unwrap(); - assert!(table_config.is_in_commit_timestamps_supported()); - assert!(table_config.is_in_commit_timestamps_enabled()); - // When ICT is enabled from table creation (version 0), it's perfectly normal - // for enablement properties to be missing - let info = table_config.in_commit_timestamp_info().unwrap(); - assert_eq!(info, InCommitTimestampInfo::Enabled { enablement: None }); - } - #[test] fn fails_on_unsupported_feature() { let schema = StructType::new_unchecked([StructField::nullable("value", DataType::INTEGER)]); let metadata = Metadata::try_new(None, None, schema, vec![], 0, HashMap::new()).unwrap(); From 2b79bebed208c31ae3195340bb560714d8ea196c Mon Sep 17 00:00:00 2001 From: DrakeLin Date: Mon, 29 Sep 2025 23:58:05 +0000 Subject: [PATCH 3/6] fix --- kernel/src/table_configuration.rs | 36 +++++++++++++++---------------- 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/kernel/src/table_configuration.rs b/kernel/src/table_configuration.rs index 6830edf56..e39e0e482 100644 --- a/kernel/src/table_configuration.rs +++ b/kernel/src/table_configuration.rs @@ -564,24 +564,6 @@ mod test { assert_eq!(info, InCommitTimestampEnablement::Enabled { enablement: Some((5, 100)) }) } #[test] - fn ict_supported_and_not_enabled() { - let schema = StructType::new_unchecked([StructField::nullable("value", DataType::INTEGER)]); - let metadata = Metadata::try_new(None, None, schema, vec![], 0, HashMap::new()).unwrap(); - let protocol = Protocol::try_new( - 3, - 7, - Some::>(vec![]), - Some([WriterFeature::InCommitTimestamp]), - ) - .unwrap(); - let table_root = Url::try_from("file:///").unwrap(); - let table_config = TableConfiguration::try_new(metadata, protocol, table_root, 0).unwrap(); - assert!(table_config.is_in_commit_timestamps_supported()); - assert!(!table_config.is_in_commit_timestamps_enabled()); - let info = table_config.in_commit_timestamp_enablement().unwrap(); - assert_eq!(info, InCommitTimestampEnablement::NotEnabled); - } - #[test] fn ict_enabled_with_partial_enablement_info() { let schema = StructType::new_unchecked([StructField::nullable("value", DataType::INTEGER)]); let metadata = Metadata::try_new( @@ -620,6 +602,24 @@ mod test { )); } #[test] + fn ict_supported_and_not_enabled() { + let schema = StructType::new_unchecked([StructField::nullable("value", DataType::INTEGER)]); + let metadata = Metadata::try_new(None, None, schema, vec![], 0, HashMap::new()).unwrap(); + let protocol = Protocol::try_new( + 3, + 7, + Some::>(vec![]), + Some([WriterFeature::InCommitTimestamp]), + ) + .unwrap(); + let table_root = Url::try_from("file:///").unwrap(); + let table_config = TableConfiguration::try_new(metadata, protocol, table_root, 0).unwrap(); + assert!(table_config.is_in_commit_timestamps_supported()); + assert!(!table_config.is_in_commit_timestamps_enabled()); + let info = table_config.in_commit_timestamp_enablement().unwrap(); + assert_eq!(info, InCommitTimestampEnablement::NotEnabled); + } + #[test] fn fails_on_unsupported_feature() { let schema = StructType::new_unchecked([StructField::nullable("value", DataType::INTEGER)]); let metadata = Metadata::try_new(None, None, schema, vec![], 0, HashMap::new()).unwrap(); From 9e4526c5678d1cf55d4bfdcb40c135e8b2833790 Mon Sep 17 00:00:00 2001 From: DrakeLin Date: Tue, 30 Sep 2025 00:01:28 +0000 Subject: [PATCH 4/6] fmt --- kernel/src/table_configuration.rs | 33 ++++++++++++++++++++----------- 1 file changed, 21 insertions(+), 12 deletions(-) diff --git a/kernel/src/table_configuration.rs b/kernel/src/table_configuration.rs index e39e0e482..8ae6ae6b8 100644 --- a/kernel/src/table_configuration.rs +++ b/kernel/src/table_configuration.rs @@ -29,10 +29,10 @@ pub(crate) enum InCommitTimestampEnablement { /// In-commit timestamps is not enabled NotEnabled, /// In-commit timestamps is enabled - Enabled { + Enabled { /// Enablement information, if available. `None` indicates the table was created /// with ICT enabled from the beginning (no enablement properties needed). - enablement: Option<(Version, i64)> + enablement: Option<(Version, i64)>, }, } @@ -330,17 +330,18 @@ impl TableConfiguration { .unwrap_or(false) } - /// Returns information about in-commit timestamp enablement state. /// /// Returns an error if only one of the enablement properties is present, as this indicates /// an inconsistent state. #[allow(unused)] - pub(crate) fn in_commit_timestamp_enablement(&self) -> DeltaResult { + pub(crate) fn in_commit_timestamp_enablement( + &self, + ) -> DeltaResult { if !self.is_in_commit_timestamps_enabled() { return Ok(InCommitTimestampEnablement::NotEnabled); } - + let enablement_version = self .table_properties() .in_commit_timestamp_enablement_version; @@ -349,8 +350,8 @@ impl TableConfiguration { .in_commit_timestamp_enablement_timestamp; match (enablement_version, enablement_timestamp) { - (Some(version), Some(timestamp)) => Ok(InCommitTimestampEnablement::Enabled { - enablement: Some((version, timestamp)) + (Some(version), Some(timestamp)) => Ok(InCommitTimestampEnablement::Enabled { + enablement: Some((version, timestamp)) }), (Some(_), None) => Err(Error::generic( "In-commit timestamp enabled, but enablement timestamp is missing while enablement version is present" @@ -358,8 +359,8 @@ impl TableConfiguration { (None, Some(_)) => Err(Error::generic( "In-commit timestamp enabled, but enablement version is missing while enablement timestamp is present" )), - (None, None) => Ok(InCommitTimestampEnablement::Enabled { - enablement: None + (None, None) => Ok(InCommitTimestampEnablement::Enabled { + enablement: None }), } } @@ -437,7 +438,7 @@ mod test { use crate::utils::test_utils::assert_result_error_with_message; use crate::Error; - use super::{TableConfiguration, InCommitTimestampEnablement}; + use super::{InCommitTimestampEnablement, TableConfiguration}; #[test] fn dv_supported_not_enabled() { @@ -522,7 +523,10 @@ mod test { // When ICT is enabled from table creation (version 0), it's perfectly normal // for enablement properties to be missing let info = table_config.in_commit_timestamp_enablement().unwrap(); - assert_eq!(info, InCommitTimestampEnablement::Enabled { enablement: None }); + assert_eq!( + info, + InCommitTimestampEnablement::Enabled { enablement: None } + ); } #[test] fn ict_supported_and_enabled() { @@ -561,7 +565,12 @@ mod test { assert!(table_config.is_in_commit_timestamps_supported()); assert!(table_config.is_in_commit_timestamps_enabled()); let info = table_config.in_commit_timestamp_enablement().unwrap(); - assert_eq!(info, InCommitTimestampEnablement::Enabled { enablement: Some((5, 100)) }) + assert_eq!( + info, + InCommitTimestampEnablement::Enabled { + enablement: Some((5, 100)) + } + ) } #[test] fn ict_enabled_with_partial_enablement_info() { From cedb8d251d3e4db58d40c5b0273859d6449fc39a Mon Sep 17 00:00:00 2001 From: DrakeLin Date: Tue, 30 Sep 2025 00:01:50 +0000 Subject: [PATCH 5/6] fmt --- kernel/src/table_configuration.rs | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/kernel/src/table_configuration.rs b/kernel/src/table_configuration.rs index 8ae6ae6b8..93773e6c5 100644 --- a/kernel/src/table_configuration.rs +++ b/kernel/src/table_configuration.rs @@ -351,17 +351,15 @@ impl TableConfiguration { match (enablement_version, enablement_timestamp) { (Some(version), Some(timestamp)) => Ok(InCommitTimestampEnablement::Enabled { - enablement: Some((version, timestamp)) + enablement: Some((version, timestamp)), }), (Some(_), None) => Err(Error::generic( - "In-commit timestamp enabled, but enablement timestamp is missing while enablement version is present" + "In-commit timestamp enabled, but enablement timestamp is missing", )), (None, Some(_)) => Err(Error::generic( - "In-commit timestamp enabled, but enablement version is missing while enablement timestamp is present" + "In-commit timestamp enabled, but enablement version is missing", )), - (None, None) => Ok(InCommitTimestampEnablement::Enabled { - enablement: None - }), + (None, None) => Ok(InCommitTimestampEnablement::Enabled { enablement: None }), } } From 2624f98f335a4fa558cca88d915fcd07a8a60603 Mon Sep 17 00:00:00 2001 From: DrakeLin Date: Tue, 30 Sep 2025 17:06:37 +0000 Subject: [PATCH 6/6] all --- kernel/src/table_configuration.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/kernel/src/table_configuration.rs b/kernel/src/table_configuration.rs index 93773e6c5..b5586d0e4 100644 --- a/kernel/src/table_configuration.rs +++ b/kernel/src/table_configuration.rs @@ -359,6 +359,8 @@ impl TableConfiguration { (None, Some(_)) => Err(Error::generic( "In-commit timestamp enabled, but enablement version is missing", )), + // If InCommitTimestamps was enabled at the beginning of the table's history, + // it may have an empty enablement version and timestamp (None, None) => Ok(InCommitTimestampEnablement::Enabled { enablement: None }), } } @@ -605,7 +607,7 @@ mod test { assert!(table_config.is_in_commit_timestamps_enabled()); assert!(matches!( table_config.in_commit_timestamp_enablement(), - Err(Error::Generic(msg)) if msg.contains("enablement timestamp is missing while enablement version is present") + Err(Error::Generic(msg)) if msg.contains("In-commit timestamp enabled, but enablement timestamp is missing") )); } #[test]