feat(catalog): adding support for purge_table#2232
Conversation
| /// When set to `false`, data files will not be deleted when a table is dropped. | ||
| /// Defaults to `true`. | ||
| pub const GC_ENABLED: &str = "gc.enabled"; | ||
| const GC_ENABLED_DEFAULT: bool = true; |
There was a problem hiding this comment.
Should be moved to TableProperty
| } | ||
|
|
||
| /// Deletes a collection of files, suppressing individual failures. | ||
| async fn delete_files<'a>(io: &FileIO, paths: impl Iterator<Item = &'a str>) { |
There was a problem hiding this comment.
Should use delete_stream api we are about to add
| /// the table does not exist. | ||
| /// - Any network or communication error occurs with the database backend. | ||
| async fn drop_table(&self, table: &TableIdent) -> Result<()> { | ||
| async fn drop_table_with_purge(&self, table: &TableIdent, _purge: bool) -> Result<()> { |
There was a problem hiding this comment.
glue should use purge
| /// the table does not exist. | ||
| /// - Any network or communication error occurs with the database backend. | ||
| async fn drop_table(&self, table: &TableIdent) -> Result<()> { | ||
| async fn drop_table_with_purge(&self, table: &TableIdent, _purge: bool) -> Result<()> { |
There was a problem hiding this comment.
hms should use purge, looking at java's impl: https://github.com/apache/iceberg/blob/8c2ca1d084fca37671ba8b38d59ea3f5a187b147/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java#L244-L251
Looks like we intend to skip hive's purge but use fileIO to purge table
| } | ||
|
|
||
| async fn drop_table(&self, identifier: &TableIdent) -> Result<()> { | ||
| async fn drop_table_with_purge(&self, identifier: &TableIdent, _purge: bool) -> Result<()> { |
There was a problem hiding this comment.
I'm actually not sure if sqlCatalog should support this
|
|
||
| /// Drop a table from the catalog. | ||
| async fn drop_table(&self, table_ident: &TableIdent) -> Result<()> { | ||
| async fn drop_table_with_purge(&self, table_ident: &TableIdent, _purge: bool) -> Result<()> { |
There was a problem hiding this comment.
Memory catalog should use purge
| /// | ||
| /// This is equivalent to calling `drop_table_with_purge(table, true)`. | ||
| async fn drop_table(&self, table: &TableIdent) -> Result<()> { | ||
| self.drop_table_with_purge(table, true).await |
There was a problem hiding this comment.
I know this is a default function, but I'm a bit concerned with forcing purge here. For instance, in the glue catalog rename leverages a drop_table() call.
iceberg-rust/crates/catalog/glue/src/catalog.rs
Lines 748 to 756 in d15deeb
This could mean that data would be incorrectly cleaned up. Same issue applies to the DataFusion integration right in this case we wouldn't want to purge on drop table (it looks like they haven't added a purge clause yet). Either way we need to either expose purge higher and force it to false or fix all these call sites (catalog, and engine level)
| async fn drop_table(&self, table: &TableIdent) -> Result<()>; | ||
| /// | ||
| /// If `purge` is true, the catalog should also delete the underlying table data. | ||
| async fn drop_table_with_purge(&self, table: &TableIdent, purge: bool) -> Result<()>; |
There was a problem hiding this comment.
the naming here makes it seem like we are always purging
| }; | ||
|
|
||
| for entry in manifest.entries() { | ||
| let _ = io.delete(entry.data_file.file_path()).await; |
There was a problem hiding this comment.
This looks like a good case to add batch delete support with all the storage work being done
2 participants
Which issue does this PR close?
What changes are included in this PR?
Are these changes tested?