feat: transactional UpdateProperties method support #321
Conversation
zhjwpku
left a comment
zhjwpku
left a comment
There was a problem hiding this comment.
This is consistent with the rest catalog open api, so +1, thanks.
src/iceberg/transaction.h
Outdated
| /// \param schema_ids the schema ids to remove | ||
| /// \return a new RemoveSchemas | ||
| virtual std::shared_ptr<RemoveSchemas> RemoveSchemas( | ||
| const std::vector<int32_t>& schema_ids) = 0; |
There was a problem hiding this comment.
| const std::vector<int32_t>& schema_ids) = 0; | |
| std::span<const int32_t> schema_ids) = 0; |
Should we use span to accept broader range of input types for here and below?
lishuxu
force-pushed
the
feature/transaction-api
branch
from
23bf661 to
0c1f156
Compare
lishuxu
changed the title
|
Could you rebase to resolve conflict? |
lishuxu
force-pushed
the
feature/transaction-api
branch
from
aa4cadb to
14bd4f8
Compare
done |
| /// \brief Set whether the last operation in a transaction has been committed | ||
| /// | ||
| /// \param committed true if the last operation has been committed, false otherwise | ||
| virtual void SetLastOperationCommitted(bool committed) = 0; |
There was a problem hiding this comment.
This function is only used by transaction catalog so it should not appear here.
src/iceberg/table.cc
Outdated
|
|
||
| std::unique_ptr<Transaction> Table::NewTransaction() const { | ||
| throw NotImplemented("Table::NewTransaction is not implemented"); | ||
| return std::make_unique<BaseTransaction>(shared_from_this(), catalog_); |
There was a problem hiding this comment.
I'd recommend directly pass Table* to it. Similarly, for catalog_ we can use catalog_.get() to just use pointer. The created transaction object should not outlive the table and catalog objects. It is unnecessary to increase the reference counter of them.
I'm open to keep the current design. WDYT @HuaHuaY?
There was a problem hiding this comment.
When the table‘s content changes, or we drop a table, what should the executing transaction hold?
src/iceberg/base_transaction.h
Outdated
| std::unique_ptr<::iceberg::UpdateProperties> UpdateProperties() override; | ||
|
|
||
| std::unique_ptr<AppendFiles> NewAppend() override; |
There was a problem hiding this comment.
| std::unique_ptr<::iceberg::UpdateProperties> UpdateProperties() override; | |
| std::unique_ptr<AppendFiles> NewAppend() override; | |
| std::shared_ptr<UpdateProperties> NewUpdateProperties() override; | |
| std::shared_ptr<AppendFiles> NewAppend() override; |
It seems that returning shared_ptr will make it easier to collect and transform the update internally. Adding New prefix also can avoid iceberg:: prefix in the return type.
There was a problem hiding this comment.
Thanks for the suggestion.
In the current transaction implementation, we don’t need to collect or retain PendingUpdate instances across the transaction lifecycle, so returning std::unique_ptr keeps the ownership semantics explicit and avoids introducing shared ownership where it isn’t required.
I agree that returning shared_ptr could make internal aggregation or transformation of updates easier if the transaction logic evolves in that direction.
src/iceberg/base_transaction.cc
Outdated
| auto update = CheckAndCreateUpdate<::iceberg::UpdateProperties>( | ||
| table_->name(), catalog_, CurrentMetadata()); | ||
| if (!update.has_value()) { | ||
| ERROR_TO_EXCEPTION(update.error()); |
There was a problem hiding this comment.
We cannot throw. Perhaps we should also use Result wrapper for these update return types.
| * For mutating calls such as UpdateTable or HasLastOperationCommitted, it delegates back | ||
| * to the owning BaseTransaction so staged updates remain private until commit. | ||
| */ | ||
| class ICEBERG_EXPORT TransactionCatalog : public Catalog { |
There was a problem hiding this comment.
Can we move this class to base_transaction.cc?
| context_.pending_updates.emplace_back(std::move(update)); | ||
| } | ||
|
|
||
| return std::make_unique<Table>( |
There was a problem hiding this comment.
Why do we need to return a table?
lishuxu
force-pushed
the
feature/transaction-api
branch
from
c3784d3 to
56802ce
Compare
6 participants
feat: transactional UpdateProperties method support
Table/Catalog produce a Transaction (e.g., NewTransaction or staged create/replace).
Clients build PendingUpdate actions (e.g., UpdateProperties, AppendFiles), each validating via Apply() (conflict checks, reserved keys, format-version parsing, schema/metrics validation).
Transaction::CommitTransaction() is expected to gather pending updates and call the catalog’s UpdateTable (with derived TableUpdate objects and requirements) in one atomic commit; on success, metadata refreshes; on failure, errors propagate