Skip to content

feat(WIP): Add TransactionAction #1400

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

Draft
wants to merge 16 commits into
base: main
Choose a base branch
from
Draft

Conversation

CTTY
Copy link
Contributor

@CTTY CTTY commented Jun 1, 2025

Err(Error::new(
ErrorKind::FeatureUnsupported,
"MemoryCatalog does not currently support updating tables.",
))
}

async fn commit_table(&self, base: &Table, current: Table) -> Result<Table> {
Copy link
Contributor Author

@CTTY CTTY Jun 1, 2025

Choose a reason for hiding this comment

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

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_tablelink.
    • 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)

@CTTY CTTY force-pushed the ctty/tx-commit branch from 87ad078 to d481d69 Compare June 1, 2025 05:24
Copy link
Contributor

@liurenjie1024 liurenjie1024 left a comment

Choose a reason for hiding this comment

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

Left some suggestions.

self
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

pub struct SetLocationAction {
   tx: Option<Transaction>
   location: Option<String>
}

impl SetLocation {
  fn apply(self) -> Result<Transaction> {
   let tx = self.tx.take();
    self.tx.add_action(Arc::new(self));
  }
}

// catalog.clone()).await
// }
// }).await

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried both tokio-retry2 and backon.

Both will require us toss around an intermediate Error type (RetryError for tokio-retry2 and anyhow::Error for backon). backon integrates better with my existing code, which requires &mut self in the do_commit method. Also, somewhat surprisingly, tokio-retry2 doesn't provide min_delay.

I've chosen to go with backon here but would appreciate any other feedback

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants