CTTY commented on code in PR #1451:
URL: https://github.com/apache/iceberg-rust/pull/1451#discussion_r2159684918
##########
crates/iceberg/src/transaction/mod.rs:
##########
@@ -45,60 +45,55 @@ use crate::{Catalog, TableCommit, TableRequirement,
TableUpdate};
/// Table transaction.
pub struct Transaction {
- base_table: Table,
- current_table: Table,
+ table: Table,
actions: Vec<BoxedTransactionAction>,
- updates: Vec<TableUpdate>,
- requirements: Vec<TableRequirement>,
}
impl Transaction {
/// Creates a new transaction.
pub fn new(table: &Table) -> Self {
Self {
- base_table: table.clone(),
- current_table: table.clone(),
+ table: table.clone(),
actions: vec![],
- updates: vec![],
- requirements: vec![],
}
}
- fn update_table_metadata(&mut self, updates: &[TableUpdate]) -> Result<()>
{
- let mut metadata_builder =
self.current_table.metadata().clone().into_builder(None);
+ fn update_table_metadata(&self, table: &mut Table, updates:
&[TableUpdate]) -> Result<()> {
+ let mut metadata_builder = table.metadata().clone().into_builder(None);
for update in updates {
metadata_builder = update.clone().apply(metadata_builder)?;
}
- self.current_table
- .with_metadata(Arc::new(metadata_builder.build()?.metadata));
+ table.with_metadata(Arc::new(metadata_builder.build()?.metadata));
Ok(())
}
fn apply(
- &mut self,
+ &self,
+ table: &mut Table,
updates: Vec<TableUpdate>,
requirements: Vec<TableRequirement>,
+ existing_updates: &mut Vec<TableUpdate>,
+ existing_requirements: &mut Vec<TableRequirement>,
) -> Result<()> {
for requirement in &requirements {
- requirement.check(Some(self.current_table.metadata()))?;
+ requirement.check(Some(table.metadata()))?;
}
- self.update_table_metadata(&updates)?;
+ self.update_table_metadata(table, &updates)?;
- self.updates.extend(updates);
+ existing_updates.extend(updates);
// For the requirements, it does not make sense to add a requirement
more than once
// For example, you cannot assert that the current schema has two
different IDs
for new_requirement in requirements {
- if self
- .requirements
+ if existing_requirements
.iter()
.map(discriminant)
.all(|d| d != discriminant(&new_requirement))
{
- self.requirements.push(new_requirement);
+ existing_requirements.push(new_requirement);
Review Comment:
I'm +1 on removing this. we should return error when requirements conflict
with each other
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
