Skip to content

Commit 6820480

Browse files
authored
feat!: add P&M to CommitMetadata and enforce committer/table type matching (delta-io#2250)
## 🥞 Stacked PR Use this [link](https://github.com/delta-io/delta-kernel-rs/pull/2250/files) to review incremental changes. - [**stack/create-table-utils-pt3**](delta-io#2250) [[Files changed](https://github.com/delta-io/delta-kernel-rs/pull/2250/files)] - [stack/create-table-utils-pt4](delta-io#2254) [[Files changed](https://github.com/delta-io/delta-kernel-rs/pull/2254/files/c71e28b2c5147bed800bf672f1d9920fff456882..e553d73653b343e65473fdba3833823d6cce74e1)] - [stack/remove-catalog-managed-flag](delta-io#2310) [[Files changed](https://github.com/delta-io/delta-kernel-rs/pull/2310/files/e553d73653b343e65473fdba3833823d6cce74e1..21b747eb65a5d99133cf1d6dfabf417564ff92b9)] --------- ## What changes are proposed in this pull request? Expands `CommitMetadata` to carry protocol/metadata state and commit change detection: - `protocol`/`metadata`: read snapshot state - `new_protocol`/`new_metadata`: present when the commit changes P&M - `domain_metadata_changes`: domain metadata actions in this commit - Public getters: `has_protocol_change()`, `has_metadata_change()`, `has_domain_metadata_change` Also adds committer/table type validations: a catalog committer can only commit to catalog-managed tables, and a non-catalog committer can only commit to non-catalog-managed tables. ## How was this change tested? 1. `disallow_catalog_committer_for_non_catalog_managed_table` -- existing non catalog managed table with catalog committer returns error 2. `disallow_catalog_committer_for_non_catalog_managed_create_table` -- create-table without catalogManaged with catalog committer returns error 3. Updated `test_commit_metadata` and `FileSystemCommitter` tests for new `CommitMetadata::new` signature 4. Existing tests pass
1 parent 6fa225e commit 6820480

6 files changed

Lines changed: 406 additions & 51 deletions

File tree

ffi/src/transaction/mod.rs

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -494,15 +494,21 @@ mod tests {
494494

495495
let tmp_test_dir = tempdir()?;
496496
let tmp_dir_local_url = Url::from_directory_path(tmp_test_dir.path()).unwrap();
497-
let partition_columns = vec![];
498497

499-
for (table_url, _engine, store, _table_name) in setup_test_tables(
498+
// Create a catalog-managed table so UCCommitter (a catalog committer) is allowed.
499+
let (store, _test_engine, table_location) =
500+
test_utils::engine_store_setup("test_uc_table", Some(&tmp_dir_local_url));
501+
let table_url = test_utils::create_table(
502+
store.clone(),
503+
table_location,
500504
schema,
501-
&partition_columns,
502-
Some(&tmp_dir_local_url),
503-
"test_uc_table",
505+
&[],
506+
true, // use v3/v7 protocol
507+
vec!["catalogManaged"],
508+
vec!["inCommitTimestamp", "catalogManaged"],
504509
)
505-
.await?
510+
.await?;
511+
506512
{
507513
let table_path = table_url.to_file_path().unwrap();
508514
let table_path_str = table_path.to_str().unwrap();

ffi/tests/test-delta-kernel-unity-catalog-ffi/run_test.sh

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,9 +4,9 @@ set -euxo pipefail
44

55
OUT_FILE=$(mktemp "${TMPDIR:-/tmp}/catalog_test.out.XXXX")
66
TMP_TABLE_DIR=$(mktemp -d "${TMPDIR:-/tmp}/catalog_test.table.XXXX")
7-
cp -r ../../../../kernel/tests/data/table-with-dv-small "$TMP_TABLE_DIR"
7+
cp -r ../../../../delta-kernel-unity-catalog/tests/data/catalog_managed_0 "$TMP_TABLE_DIR"
88

9-
./delta_kernel_unity_catalog_example "$TMP_TABLE_DIR/table-with-dv-small"
9+
./delta_kernel_unity_catalog_example "$TMP_TABLE_DIR/catalog_managed_0"
1010
CATALOG_EXIT_CODE=$?
1111

1212
rm "$OUT_FILE"

kernel/src/committer/commit_types.rs

Lines changed: 165 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,105 @@
11
//! Commit metadata types for the committer module.
22
3+
#[cfg(any(test, feature = "test-utils"))]
4+
use std::collections::HashMap;
5+
#[cfg(any(test, feature = "test-utils"))]
6+
use std::sync::Arc;
7+
38
use url::Url;
49

10+
use crate::actions::{Metadata, Protocol};
511
use crate::path::LogRoot;
612
use crate::{DeltaResult, Version};
713

8-
/// `CommitMetadata` bundles the metadata about a commit operation. This currently includes the
9-
/// commit path and version but will expand to things like `Protocol`, `Metadata`, etc. to allow
10-
/// for catalogs to understand/cache/persist more information about the table at commit time.
14+
/// The type of commit operation being performed. This communicates to the committer whether this
15+
/// is a table creation or a write to an existing table, and whether the table is catalog-managed.
16+
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
17+
pub enum CommitType {
18+
/// Creating a new table via filesystem (no catalog involvement).
19+
PathBasedCreate,
20+
/// Creating a new catalog-managed table.
21+
CatalogManagedCreate,
22+
/// Writing to an existing path-based table.
23+
PathBasedWrite,
24+
/// Writing to an existing catalog-managed table.
25+
CatalogManagedWrite,
26+
// TODO: Wire these up when ALTER TABLE SET TBLPROPERTIES is supported.
27+
/// Upgrading an existing path-based table to catalog-managed. Not currently supported.
28+
#[allow(dead_code)]
29+
UpgradeToCatalogManaged,
30+
/// Downgrading an existing catalog-managed table to path-based. Not currently supported.
31+
#[allow(dead_code)]
32+
DowngradeToPathBased,
33+
}
34+
35+
impl CommitType {
36+
/// Returns `true` if this is a create-table commit (version 0).
37+
pub fn is_create(&self) -> bool {
38+
matches!(self, Self::PathBasedCreate | Self::CatalogManagedCreate)
39+
}
40+
41+
/// Returns `true` if this commit includes a catalog-managed operation,
42+
/// including upgrade/downgrade.
43+
pub fn requires_catalog_committer(&self) -> bool {
44+
matches!(
45+
self,
46+
Self::CatalogManagedCreate
47+
| Self::CatalogManagedWrite
48+
| Self::UpgradeToCatalogManaged
49+
| Self::DowngradeToPathBased
50+
)
51+
}
52+
}
53+
54+
/// The protocol and metadata state for this commit. Groups the read snapshot state (if any)
55+
/// and the new state being committed (if any).
56+
#[derive(Debug)]
57+
#[allow(dead_code)] // Fields read by delta-kernel-unity-catalog via effective_protocol/metadata
58+
pub(crate) struct CommitProtocolMetadata {
59+
/// Existing table protocol from read snapshot. `None` for create-table.
60+
read_protocol: Option<Protocol>,
61+
/// Existing table metadata from read snapshot. `None` for create-table.
62+
read_metadata: Option<Metadata>,
63+
/// New protocol being committed. `Some` for create-table and future ALTER TABLE.
64+
new_protocol: Option<Protocol>,
65+
/// New metadata being committed. `Some` for create-table and future ALTER TABLE.
66+
new_metadata: Option<Metadata>,
67+
}
68+
69+
impl CommitProtocolMetadata {
70+
pub(crate) fn try_new(
71+
read_protocol: Option<Protocol>,
72+
read_metadata: Option<Metadata>,
73+
new_protocol: Option<Protocol>,
74+
new_metadata: Option<Metadata>,
75+
) -> DeltaResult<Self> {
76+
if read_protocol.is_some() != read_metadata.is_some() {
77+
return Err(crate::Error::generic(
78+
"read_protocol and read_metadata must both be present or both be absent",
79+
));
80+
}
81+
if read_protocol.is_none() && new_protocol.is_none() {
82+
return Err(crate::Error::generic(
83+
"CommitProtocolMetadata requires at least one protocol (read or new)",
84+
));
85+
}
86+
if read_metadata.is_none() && new_metadata.is_none() {
87+
return Err(crate::Error::generic(
88+
"CommitProtocolMetadata requires at least one metadata (read or new)",
89+
));
90+
}
91+
Ok(Self {
92+
read_protocol,
93+
read_metadata,
94+
new_protocol,
95+
new_metadata,
96+
})
97+
}
98+
}
99+
100+
/// `CommitMetadata` bundles the metadata about a commit operation. This includes the commit path,
101+
/// version, and protocol/metadata state of the table being committed to. Catalog committers can
102+
/// use the protocol and metadata getters to validate or inspect the commit.
11103
///
12104
/// Note that this struct cannot be constructed. It is handed to the [`Committer`] (in the
13105
/// [`commit`] method) by the kernel when a transaction is being committed.
@@ -21,23 +113,30 @@ use crate::{DeltaResult, Version};
21113
pub struct CommitMetadata {
22114
pub(crate) log_root: LogRoot,
23115
pub(crate) version: Version,
116+
pub(crate) commit_type: CommitType,
24117
pub(crate) in_commit_timestamp: i64,
25118
pub(crate) max_published_version: Option<Version>,
26-
// in the future this will include Protocol, Metadata, CommitInfo, Domain Metadata, etc.
119+
/// Protocol and metadata state for this commit.
120+
#[allow(dead_code)] // Read by delta-kernel-unity-catalog for commit validation
121+
pub(crate) protocol_metadata: CommitProtocolMetadata,
27122
}
28123

29124
impl CommitMetadata {
30125
pub(crate) fn new(
31126
log_root: LogRoot,
32127
version: Version,
128+
commit_type: CommitType,
33129
in_commit_timestamp: i64,
34130
max_published_version: Option<Version>,
131+
protocol_metadata: CommitProtocolMetadata,
35132
) -> Self {
36133
Self {
37134
log_root,
38135
version,
136+
commit_type,
39137
in_commit_timestamp,
40138
max_published_version,
139+
protocol_metadata,
41140
}
42141
}
43142

@@ -62,6 +161,11 @@ impl CommitMetadata {
62161
self.version
63162
}
64163

164+
/// The type of commit operation being performed.
165+
pub fn commit_type(&self) -> CommitType {
166+
self.commit_type
167+
}
168+
65169
/// The in-commit timestamp for the commit. Note that this may differ from the actual commit
66170
/// file modification time.
67171
pub fn in_commit_timestamp(&self) -> i64 {
@@ -73,15 +177,58 @@ impl CommitMetadata {
73177
self.max_published_version
74178
}
75179

180+
/// The root URL of the table being committed to.
76181
pub fn table_root(&self) -> &Url {
77182
self.log_root.table_root()
78183
}
79184

185+
/// Returns the effective protocol for this commit. Prefers new_protocol (create-table / ALTER
186+
/// TABLE), falling back to the read snapshot's protocol.
187+
#[allow(dead_code)] // Used by delta-kernel-unity-catalog for commit validation
188+
pub(crate) fn effective_protocol(&self) -> DeltaResult<&Protocol> {
189+
let pm = &self.protocol_metadata;
190+
pm.new_protocol
191+
.as_ref()
192+
.or(pm.read_protocol.as_ref())
193+
.ok_or_else(|| {
194+
crate::Error::internal_error(
195+
"CommitProtocolMetadata should have at least one protocol",
196+
)
197+
})
198+
}
199+
200+
/// Returns the effective metadata for this commit. Prefers new_metadata (create-table / ALTER
201+
/// TABLE), falling back to the read snapshot's metadata.
202+
#[allow(dead_code)] // Used by delta-kernel-unity-catalog for commit validation
203+
pub(crate) fn effective_metadata(&self) -> DeltaResult<&Metadata> {
204+
let pm = &self.protocol_metadata;
205+
pm.new_metadata
206+
.as_ref()
207+
.or(pm.read_metadata.as_ref())
208+
.ok_or_else(|| {
209+
crate::Error::internal_error(
210+
"CommitProtocolMetadata should have at least one metadata",
211+
)
212+
})
213+
}
214+
80215
/// Creates a new `CommitMetadata` for the given `table_root` and `version`. Test-only.
216+
///
217+
/// Uses a default modern protocol (empty features) and empty metadata.
81218
#[cfg(any(test, feature = "test-utils"))]
82219
pub fn new_unchecked(table_root: Url, version: Version) -> DeltaResult<Self> {
83220
let log_root = crate::path::LogRoot::new(table_root)?;
84-
Ok(Self::new(log_root, version, 0, None))
221+
let protocol = Protocol::try_new_modern(Vec::<&str>::new(), Vec::<&str>::new())?;
222+
let schema = Arc::new(crate::schema::StructType::new_unchecked(vec![]));
223+
let metadata = Metadata::try_new(None, None, schema, vec![], 0, HashMap::new())?;
224+
Ok(Self::new(
225+
log_root,
226+
version,
227+
CommitType::PathBasedWrite,
228+
0,
229+
None,
230+
CommitProtocolMetadata::try_new(Some(protocol), Some(metadata), None, None)?,
231+
))
85232
}
86233
}
87234

@@ -104,6 +251,8 @@ pub enum CommitResponse {
104251
mod tests {
105252
use super::*;
106253

254+
use std::sync::Arc;
255+
107256
use crate::path::LogRoot;
108257
use url::Url;
109258

@@ -114,8 +263,18 @@ mod tests {
114263
let version = 42;
115264
let ts = 1234;
116265
let max_published_version = Some(42);
266+
let protocol = Protocol::try_new_modern(Vec::<&str>::new(), Vec::<&str>::new()).unwrap();
267+
let schema = Arc::new(crate::schema::StructType::new_unchecked(vec![]));
268+
let metadata = Metadata::try_new(None, None, schema, vec![], 0, HashMap::new()).unwrap();
117269

118-
let commit_metadata = CommitMetadata::new(log_root, version, ts, max_published_version);
270+
let commit_metadata = CommitMetadata::new(
271+
log_root,
272+
version,
273+
CommitType::PathBasedWrite,
274+
ts,
275+
max_published_version,
276+
CommitProtocolMetadata::try_new(Some(protocol), Some(metadata), None, None).unwrap(),
277+
);
119278

120279
// version
121280
assert_eq!(commit_metadata.version(), 42);

kernel/src/committer/filesystem.rs

Lines changed: 37 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -87,8 +87,11 @@ impl Committer for FileSystemCommitter {
8787
mod tests {
8888
use super::*;
8989

90+
use std::collections::HashMap;
9091
use std::sync::Arc;
9192

93+
use crate::actions::{Metadata, Protocol};
94+
use crate::committer::{CommitProtocolMetadata, CommitType};
9295
use crate::engine::default::DefaultEngineBuilder;
9396
use crate::object_store::memory::InMemory;
9497
use crate::object_store::path::Path;
@@ -124,7 +127,7 @@ mod tests {
124127
.unwrap_err();
125128
assert!(matches!(
126129
err,
127-
crate::Error::Generic(e) if e.contains("A catalog committer must be used to commit to catalog-managed tables. Please provide a catalog committer via Snapshot::transaction().")
130+
crate::Error::Generic(e) if e.contains("This table is catalog-managed and requires a catalog committer.")
128131
));
129132
}
130133

@@ -136,7 +139,17 @@ mod tests {
136139

137140
let committer = FileSystemCommitter::new();
138141
let log_root = LogRoot::new(table_root).unwrap();
139-
let commit_metadata = CommitMetadata::new(log_root, 1, 12345, Some(0));
142+
let protocol = Protocol::try_new_modern(Vec::<&str>::new(), Vec::<&str>::new()).unwrap();
143+
let schema = Arc::new(crate::schema::StructType::new_unchecked(vec![]));
144+
let metadata = Metadata::try_new(None, None, schema, vec![], 0, HashMap::new()).unwrap();
145+
let commit_metadata = CommitMetadata::new(
146+
log_root,
147+
1,
148+
CommitType::PathBasedWrite,
149+
12345,
150+
Some(0),
151+
CommitProtocolMetadata::try_new(Some(protocol), Some(metadata), None, None).unwrap(),
152+
);
140153
let actions = Box::new(std::iter::empty());
141154

142155
let result = committer.commit(&engine, actions, commit_metadata).unwrap();
@@ -161,10 +174,28 @@ mod tests {
161174
let engine = DefaultEngineBuilder::new(storage).build();
162175

163176
let committer = FileSystemCommitter::new();
164-
let first_metadata =
165-
CommitMetadata::new(LogRoot::new(table_root.clone()).unwrap(), 1, 12345, Some(0));
166-
let second_metadata =
167-
CommitMetadata::new(LogRoot::new(table_root).unwrap(), 1, 12346, Some(0));
177+
let protocol = Protocol::try_new_modern(Vec::<&str>::new(), Vec::<&str>::new()).unwrap();
178+
let schema = Arc::new(crate::schema::StructType::new_unchecked(vec![]));
179+
let metadata1 =
180+
Metadata::try_new(None, None, schema.clone(), vec![], 0, HashMap::new()).unwrap();
181+
let metadata2 = Metadata::try_new(None, None, schema, vec![], 0, HashMap::new()).unwrap();
182+
let first_metadata = CommitMetadata::new(
183+
LogRoot::new(table_root.clone()).unwrap(),
184+
1,
185+
CommitType::PathBasedWrite,
186+
12345,
187+
Some(0),
188+
CommitProtocolMetadata::try_new(Some(protocol.clone()), Some(metadata1), None, None)
189+
.unwrap(),
190+
);
191+
let second_metadata = CommitMetadata::new(
192+
LogRoot::new(table_root).unwrap(),
193+
1,
194+
CommitType::PathBasedWrite,
195+
12346,
196+
Some(0),
197+
CommitProtocolMetadata::try_new(Some(protocol), Some(metadata2), None, None).unwrap(),
198+
);
168199

169200
let first = committer
170201
.commit(&engine, Box::new(std::iter::empty()), first_metadata)

kernel/src/committer/mod.rs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,8 @@ mod commit_types;
2929
mod filesystem;
3030
mod publish_types;
3131

32-
pub use commit_types::{CommitMetadata, CommitResponse};
32+
pub(crate) use commit_types::CommitProtocolMetadata;
33+
pub use commit_types::{CommitMetadata, CommitResponse, CommitType};
3334
pub use filesystem::FileSystemCommitter;
3435
pub use publish_types::{CatalogCommit, PublishMetadata};
3536

0 commit comments

Comments
 (0)