feat(catalog): Implement update_table for MemoryCatalog#1549
feat(catalog): Implement update_table for MemoryCatalog#1549liurenjie1024 merged 23 commits intoapache:mainfrom
update_table for MemoryCatalog#1549Conversation
Signed-off-by: CTTY <yxchang@amazon.com> DerGut <jannik.steinmann@gmx.de>
liurenjie1024
left a comment
liurenjie1024
left a comment
There was a problem hiding this comment.
Thanks @CTTY for this pr! Generally looks good, but I would suggest to move the refactoring of MetadataLocation to a separate pr.
| .metadata() | ||
| .write_to( | ||
| staged_table.file_io(), | ||
| staged_table.metadata_location().unwrap(), |
There was a problem hiding this comment.
We should not panic here. Also I think we should generate a new table metadata location?
There was a problem hiding this comment.
The new metadata location would be generated in TableCommit::apply and attached to the staged table
There was a problem hiding this comment.
The mod name utils is too generic. We should rename it to metadata_location or sth more meaningful.
There was a problem hiding this comment.
Also I would suggest to move this into a separate pr.
There was a problem hiding this comment.
How about we only move the usages of MetadataLocation in catalogs other than MemoryCatalog to a different PR, and have MemoryCatalog switch to use MetadataLocation in this PR?
Because we need to use MetadataLocation::with_next_version to bump metadata location version when updating a table, and it would require some cumbersome + temporary logic if without MetadataLocation
There was a problem hiding this comment.
I have removed MetadataLocation usages for catalogs other than MemoryCatalog
There was a problem hiding this comment.
I still prefer to move it to another pr, but I don't have a strong opinion on it. Currently changes also looks good to me.
Co-authored-by: Renjie Liu <liurenjie2008@gmail.com>
Co-authored-by: Renjie Liu <liurenjie2008@gmail.com>
DerGut
left a comment
DerGut
left a comment
There was a problem hiding this comment.
Thanks for taking this back on! I only found some minor issues around comments. Looks good to me otherwise 👏
|
We have just one minor change to resolve. |
Co-authored-by: Jannik Steinmann <jannik.steinmann@datadoghq.com>
Co-authored-by: Jannik Steinmann <jannik.steinmann@datadoghq.com>
Co-authored-by: Renjie Liu <liurenjie2008@gmail.com>
liurenjie1024
left a comment
liurenjie1024
left a comment
There was a problem hiding this comment.
Thanks @CTTY for this pr, LGTM!
3 participants
Which issue does this PR close?
What changes are included in this PR?
update_tableforMemoryCatalogupdate_tableimpl from feat(catalog): Implement MemoryCatalog's table update/ commit path #1405MetadataLocationParserMetadataLocationParserfor multiple catalogs, and remove the previous metadata location generatorTableCommit::applythat causes it not updatingMetadataLogAre these changes tested?
Added unit tests