CTTY commented on code in PR #1400:
URL: https://github.com/apache/iceberg-rust/pull/1400#discussion_r2118723879
##########
crates/catalog/memory/src/catalog.rs:
##########
@@ -277,12 +278,75 @@ impl Catalog for MemoryCatalog {
}
/// Update a table to the catalog.
- async fn update_table(&self, _commit: TableCommit) -> Result<Table> {
+ async fn update_table(&self, commit: TableCommit) -> Result<Table> {
Err(Error::new(
ErrorKind::FeatureUnsupported,
"MemoryCatalog does not currently support updating tables.",
))
}
+
+ async fn commit_table(&self, base: &Table, current: Table) ->
Result<Table> {
Review Comment:
I'm trying to replace `update_table` with `commit_table`, also would love
some feedback on this:
- Do we actually need `TableCommit` for catalog to commit tables?
- In the `Transaction` we have already staged changes to the
`current_table`[link](https://github.com/apache/iceberg-rust/blob/main/crates/iceberg/src/transaction/mod.rs#L42).
- `Transaction` will need to hold `Vec<TransactionAction>` so tx itself
can rebase updates easily, if needed.
- Catalogs will need to check if metadata location match and retry based
on the check result, `TableCommit` may be useful in this case, but the logic to
apply updates on the base table is still duplicate to `Transaction::apply()`
(https://github.com/apache/iceberg-rust/blob/main/crates/iceberg/src/transaction/mod.rs#L70)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
