Skip to content

refactor: Add FileIO::remove_dir_all to deprecate remove_dir #1275

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.

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 3 commits into from
May 15, 2025
Merged
Changes from all commits
Commits
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
34 changes: 32 additions & 2 deletions crates/iceberg/src/io/file_io.rs
Original file line number Diff line number Diff line change
Expand Up @@ -95,11 +95,33 @@ impl FileIO {
/// # Arguments
///
/// * path: It should be *absolute* path starting with scheme string used to construct [`FileIO`].
#[deprecated(note = "use remove_dir_all instead", since = "0.4.0")]
pub async fn remove_all(&self, path: impl AsRef<str>) -> Result<()> {
let (op, relative_path) = self.inner.create_operator(&path)?;
Ok(op.remove_all(relative_path).await?)
}

/// Remove the path and all nested dirs and files recursively.
///
/// # Arguments
///
/// * path: It should be *absolute* path starting with scheme string used to construct [`FileIO`].
///
/// # Behavior
///
/// - If the path is a file or not exist, this function will be no-op.
/// - If the path is a empty directory, this function will remove the directory itself.
/// - If the path is a non-empty directory, this function will remove the directory and all nested files and directories.
pub async fn remove_dir_all(&self, path: impl AsRef<str>) -> Result<()> {
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. It would be better to add doc to explain the behavior, e.g. what if the dir doesn't exist, what if the dir is empty.
  2. Add test case for path not ending with "/"

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe also add a test for if the path (without ending in /) refers to a file instead of dir. should be no-op

Copy link
Contributor

Choose a reason for hiding this comment

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

is this a breaking change? i.e. something we should warn users about on the next release

Copy link
Contributor

Choose a reason for hiding this comment

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

+1, we could deprecate the old one, and recommend the new api.

let (op, relative_path) = self.inner.create_operator(&path)?;
let path = if relative_path.ends_with('/') {
relative_path.to_string()
} else {
format!("{relative_path}/")
};
Ok(op.remove_all(&path).await?)
}

/// Check file exists.
///
/// # Arguments
Expand Down Expand Up @@ -441,7 +463,15 @@ mod tests {
let file_io = create_local_file_io();
assert!(file_io.exists(&a_path).await.unwrap());

file_io.remove_all(&sub_dir_path).await.unwrap();
// Remove a file should be no-op.
file_io.remove_dir_all(&a_path).await.unwrap();
assert!(file_io.exists(&a_path).await.unwrap());

// Remove a not exist dir should be no-op.
file_io.remove_dir_all("not_exists/").await.unwrap();

// Remove a dir should remove all files in it.
file_io.remove_dir_all(&sub_dir_path).await.unwrap();
assert!(!file_io.exists(&b_path).await.unwrap());
assert!(!file_io.exists(&c_path).await.unwrap());
assert!(file_io.exists(&a_path).await.unwrap());
Expand All @@ -460,7 +490,7 @@ mod tests {
let file_io = create_local_file_io();
assert!(!file_io.exists(&full_path).await.unwrap());
assert!(file_io.delete(&full_path).await.is_ok());
assert!(file_io.remove_all(&full_path).await.is_ok());
assert!(file_io.remove_dir_all(&full_path).await.is_ok());
}

#[tokio::test]
Expand Down
Loading