CTTY commented on code in PR #1448:
URL: https://github.com/apache/iceberg-rust/pull/1448#discussion_r2151121670
##########
crates/iceberg/src/transaction/append.rs:
##########
@@ -87,76 +169,45 @@ impl FastAppendAction {
/// Specifically, schema compatibility checks and support for adding to
partitioned tables
/// have not yet been implemented.
#[allow(dead_code)]
- async fn add_parquet_files(mut self, file_path: Vec<String>) ->
Result<Transaction> {
- if !self
- .snapshot_produce_action
- .tx
- .current_table
- .metadata()
- .default_spec
- .is_unpartitioned()
- {
+ async fn add_parquet_files(self, table: &Table, file_path: Vec<String>) ->
Result<Self> {
Review Comment:
Because `FastAppendAction` no longer holds `Transaction` or `Table`, we have
to pass `&Table` to this method to convert parquet files to data files. I think
1) no transaction action API should allow users to pass `Table` in to keep APIs
clear and simple, and 2) Transaction is probably a wrong place to do this file
conversion
So I'm thinking of isolating the path-to-DataFile conversion logic out to
make the API simpler. like this
```
let tx = Transaction::new()
let fast_append = tx.fast_append()
let data_files = to_data_files(vec![parquet_file_path]) // add a new helper
to convert parquet files to DataFile
let tx = fast_append.add_data_files(data_files).apply()
tx.commit
```
--
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]
