feat!(catalog): adding support for purge_table#2232
feat!(catalog): adding support for purge_table#2232CTTY wants to merge 10 commits intoapache:mainfrom
Conversation
CTTY
changed the title
| /// S3 Tables data is managed by the service, so this just delegates | ||
| /// to `drop_table` | ||
| async fn purge_table(&self, table: &TableIdent) -> Result<()> { | ||
| self.drop_table(table).await |
There was a problem hiding this comment.
I think we should throw error here. If s3table doesn't support purge, we should report it rather than silently fail
There was a problem hiding this comment.
S3 Tables doesn't support purge=false, so drop_table and purge_table should behave the same.
The "correct" way would be throwing error in drop_table and I think that's too inconvenient
There was a problem hiding this comment.
I prefer to throw error in drop_table. It's a little incovenient, but it will not surprise user.
|
|
||
| /// Reads manifests concurrently and deletes the data files referenced within. | ||
| async fn delete_data_files(io: &FileIO, manifest_paths: &HashSet<String>) -> Result<()> { | ||
| stream::iter(manifest_paths.iter().map(Ok)) |
There was a problem hiding this comment.
This is executing concurrently, not parallelly. What we need is runtime api.
There was a problem hiding this comment.
Anyway, we could fix this after runtime api is ready.
| /// S3 Tables data is managed by the service, so this just delegates | ||
| /// to `drop_table` | ||
| async fn purge_table(&self, table: &TableIdent) -> Result<()> { | ||
| self.drop_table(table).await |
There was a problem hiding this comment.
I prefer to throw error in drop_table. It's a little incovenient, but it will not surprise user.
3 participants
Which issue does this PR close?
Catalog#2133What changes are included in this PR?
purge_tabletoCatalogtrait and add default implementationAre these changes tested?
Added new tests in table_suite