Skip to content

Commit 47d990e

Browse files
authored
chore: Add assertion for empty data files for append action (#1301)
## What changes are included in this PR? The general usage to append data files is ```rust let mut action = txn.fast_append(/*commit_uuid=*/ None, /*key_metadata=*/ vec![])?; action.add_data_files(new_data_files)?; let txn = action.apply().await?; let table = txn.commit(catalog).await? ``` I found if ` new_data_files` is empty by accident, a new manifest file will still be created and tracked by manifest list file, which doesn't make sense to me (let me if team thinks it's a valid and ideal use case). In this PR, I did a few changes: - Add a new invalid argument error code; - Add assertion to fail empty data files append, and empty action commit use cases. ## Are these changes tested? Existing unit tests pass, and add a validation test for invalid data file append.
1 parent 7e36243 commit 47d990e

File tree

3 files changed

+20
-0
lines changed

3 files changed

+20
-0
lines changed

crates/iceberg/src/error.rs

+4
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,9 @@ pub type Result<T> = std::result::Result<T, Error>;
2828
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
2929
#[non_exhaustive]
3030
pub enum ErrorKind {
31+
/// The operation was rejected because the system is not in a state required for the operation’s execution.
32+
PreconditionFailed,
33+
3134
/// Iceberg don't know what happened here, and no actions other than
3235
/// just returning it back. For example, iceberg returns an internal
3336
/// service error.
@@ -76,6 +79,7 @@ impl From<ErrorKind> for &'static str {
7679
ErrorKind::TableNotFound => "TableNotFound",
7780
ErrorKind::NamespaceAlreadyExists => "NamespaceAlreadyExists",
7881
ErrorKind::NamespaceNotFound => "NamespaceNotFound",
82+
ErrorKind::PreconditionFailed => "PreconditionFailed",
7983
}
8084
}
8185
}

crates/iceberg/src/transaction/append.rs

+9
Original file line numberDiff line numberDiff line change
@@ -219,6 +219,15 @@ mod tests {
219219
use crate::transaction::Transaction;
220220
use crate::{TableRequirement, TableUpdate};
221221

222+
#[tokio::test]
223+
async fn test_empty_data_append_action() {
224+
let table = make_v2_minimal_table();
225+
let tx = Transaction::new(&table);
226+
let mut action = tx.fast_append(None, vec![]).unwrap();
227+
action.add_data_files(vec![]).unwrap();
228+
assert!(action.apply().await.is_err());
229+
}
230+
222231
#[tokio::test]
223232
async fn test_fast_append_action() {
224233
let table = make_v2_minimal_table();

crates/iceberg/src/transaction/snapshot.rs

+7
Original file line numberDiff line numberDiff line change
@@ -172,6 +172,13 @@ impl<'a> SnapshotProduceAction<'a> {
172172
// Write manifest file for added data files and return the ManifestFile for ManifestList.
173173
async fn write_added_manifest(&mut self) -> Result<ManifestFile> {
174174
let added_data_files = std::mem::take(&mut self.added_data_files);
175+
if added_data_files.is_empty() {
176+
return Err(Error::new(
177+
ErrorKind::PreconditionFailed,
178+
"No added data files found when write a manifest file",
179+
));
180+
}
181+
175182
let snapshot_id = self.snapshot_id;
176183
let format_version = self.tx.current_table.metadata().format_version();
177184
let manifest_entries = added_data_files.into_iter().map(|data_file| {

0 commit comments

Comments
 (0)