Skip to content

chore: Add assertion for empty data files for append action #1301

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
May 10, 2025
Merged

chore: Add assertion for empty data files for append action #1301

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3c21958
Add assertion for empty data files for append action
dentiny May 8, 2025
0e1b714
update error message
dentiny May 9, 2025
52b8256
use failed precondition
dentiny May 10, 2025
5b9aaf9
fix unit test
dentiny May 10, 2025
8e1f3b5
Merge branch 'main' into hjiang/add-assertion-for-empty-data-files
dentiny May 10, 2025
9a0b105
rename error kind to confirm naming convension
dentiny May 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions crates/iceberg/src/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,9 @@ pub type Result<T> = std::result::Result<T, Error>;
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
#[non_exhaustive]
pub enum ErrorKind {
/// Function receives an invalid argument, which iceberg library cannot handle.
InvalidArgument,

/// Iceberg don't know what happened here, and no actions other than
/// just returning it back. For example, iceberg returns an internal
/// service error.
Expand Down Expand Up @@ -76,6 +79,7 @@ impl From<ErrorKind> for &'static str {
ErrorKind::TableNotFound => "TableNotFound",
ErrorKind::NamespaceAlreadyExists => "NamespaceAlreadyExists",
ErrorKind::NamespaceNotFound => "NamespaceNotFound",
ErrorKind::InvalidArgument => "InvalidArgument",
}
}
}
Expand Down
8 changes: 8 additions & 0 deletions crates/iceberg/src/transaction/append.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,14 @@ mod tests {
use crate::transaction::Transaction;
use crate::{TableRequirement, TableUpdate};

#[tokio::test]
async fn test_empty_data_append_action() {
let table = make_v2_minimal_table();
let tx = Transaction::new(&table);
let mut action = tx.fast_append(None, vec![]).unwrap();
assert!(action.add_data_files(vec![]).is_err());
}

#[tokio::test]
async fn test_fast_append_action() {
let table = make_v2_minimal_table();
Expand Down
14 changes: 14 additions & 0 deletions crates/iceberg/src/transaction/snapshot.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,13 @@ impl<'a> SnapshotProduceAction<'a> {
data_files: impl IntoIterator<Item = DataFile>,
) -> Result<&mut Self> {
let data_files: Vec<DataFile> = data_files.into_iter().collect();
if data_files.is_empty() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think returning a DataInvalid error is good enough here, I don't know if invalid argument is needed. cc @Xuanwo

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error description reads to me as "data corruption".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that makes sense, InvalidArgument or maybe InvalidInput is more intuitive.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems safe to just return Ok(()) while input iterator is empty. Returning error here seems no contribute for users' experrence. Users can't take actions on this error.

They have to add something like:

if input_data.is_empty() {
    return Ok(())
}
xxx.add_data_files(input_data).await?;

So how about we just check this and early return it?

Copy link
Contributor Author

@dentiny dentiny May 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the given iterator is empty, we will end up with a manifest file with no manifest entries, with no warning / error happening (it's exactly the same behavior you described), this is something I would like to avoid.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In other words, I think having an empty manifest file doesn't make too much sense.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the given iterator is empty, we will end up with a manifest file with no manifest entries, with no warning / error happening (it's exactly the same behavior you described), this is something I would like to avoid.

This is valid and I do agree that we should not generate an empty manifest file.

However, the add_data_files function itself should be able to handle empty input safely, as users might use it in loops or pipelines with filters, where encountering an empty iterator is possible.

Therefore, I prefer to perform this check when committing the manifest file, rather than within add_data_files.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally, I want to note that add_data_files can be called multiple times and is not directly mapped to a manifest file.

return Err(Error::new(
ErrorKind::InvalidArgument,
"No data files were provided when adding data files to snapshot",
));
}

for data_file in &data_files {
if data_file.content_type() != crate::spec::DataContentType::Data {
return Err(Error::new(
Expand Down Expand Up @@ -172,6 +179,13 @@ impl<'a> SnapshotProduceAction<'a> {
// Write manifest file for added data files and return the ManifestFile for ManifestList.
async fn write_added_manifest(&mut self) -> Result<ManifestFile> {
let added_data_files = std::mem::take(&mut self.added_data_files);
if added_data_files.is_empty() {
return Err(Error::new(
ErrorKind::Unexpected,
"No added data files found when write a manifest file",
));
}

let snapshot_id = self.snapshot_id;
let format_version = self.tx.current_table.metadata().format_version();
let manifest_entries = added_data_files.into_iter().map(|data_file| {
Expand Down
Loading