liurenjie1024 commented on code in PR #1451:
URL: https://github.com/apache/iceberg-rust/pull/1451#discussion_r2158782394
##########
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:
We should move this check to start of this method. Also I don't think we
should filter here, we should return error.
##########
crates/iceberg/src/transaction/mod.rs:
##########
@@ -129,8 +124,10 @@ impl Transaction {
}
};
let mut snapshot_id = generate_random_id();
+
+ // todo revisit this, this check won't be 100% valid because it's not
checking the staging table
while self
- .current_table
+ .table
.metadata()
.snapshots()
.any(|s| s.snapshot_id() == snapshot_id)
Review Comment:
+1, I also think we should move this to `SnapshotProducer.`
##########
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:
In fact, I'm not sure if we actually need this check, maybe we could remove
this for now?
##########
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,
Review Comment:
Ditto.
##########
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<()> {
Review Comment:
We don't need `self` here?
##########
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));
Review Comment:
Not related to this pr, and it's not mandatory. But I think it's better to
change the `with_metadata(mut self)` to consume table rather than modify it.
--
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
