You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "lishuxu (via GitHub)" <gi...@apache.org> on 2025/10/28 12:33:20 UTC
[PR] feat: add table create/replace/update interface to catalog [iceberg-cpp]
lishuxu opened a new pull request, #281:
URL: https://github.com/apache/iceberg-cpp/pull/281
(no comment)
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: add table create/replace/update interface to catalog [iceberg-cpp]
Posted by "lishuxu (via GitHub)" <gi...@apache.org>.
lishuxu commented on code in PR #281:
URL: https://github.com/apache/iceberg-cpp/pull/281#discussion_r2512548844
##########
src/iceberg/catalog.h:
##########
@@ -223,6 +247,16 @@ class ICEBERG_EXPORT Catalog {
///
/// \return the Transaction to create the table
virtual std::unique_ptr<Transaction> StageCreate() = 0;
+
+ /// \brief Starts a transaction to replace the table
+ ///
+ /// \return the Transaction to replace the table
+ virtual std::unique_ptr<Transaction> StageReplace() = 0;
+
+ /// \brief Starts a transaction to create or replace the table
+ ///
+ /// \return the Transaction to create or replace the table
+ virtual std::unique_ptr<Transaction> StageCreateOrReplace() = 0;
Review Comment:
Refer to (https://github.com/apache/iceberg-python/issues/281), the replace table API is currently not planned to be supported.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: add table create/replace/update interface to catalog [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #281:
URL: https://github.com/apache/iceberg-cpp/pull/281#discussion_r2509283337
##########
src/iceberg/transaction.h:
##########
@@ -42,12 +44,23 @@ class ICEBERG_EXPORT Transaction {
/// \return a new AppendFiles
virtual std::shared_ptr<AppendFiles> NewAppend() = 0;
+ /// \brief Apply multiple metadata updates to this transaction
+ ///
+ /// \param requirements the table requirements to validate
+ /// \param updates the table updates to apply
+ /// \return Status::OK if all updates were queued successfully
+ virtual Status UpdateTable(
+ const std::vector<std::unique_ptr<TableRequirement>>& requirements,
+ std::vector<std::unique_ptr<TableUpdate>> updates) = 0;
Review Comment:
```suggestion
const std::vector<std::unique_ptr<TableUpdate>>& updates) = 0;
```
##########
src/iceberg/table.h:
##########
@@ -109,6 +109,11 @@ class ICEBERG_EXPORT Table {
/// filter data.
virtual std::unique_ptr<TableScanBuilder> NewScan() const;
+ /// \brief Create a new transaction for this table
+ ///
+ /// \return a shared pointer to the new Transaction
+ virtual std::shared_ptr<Transaction> NewTransaction() const;
Review Comment:
```suggestion
virtual std::unique_ptr<Transaction> NewTransaction() const;
```
##########
src/iceberg/catalog.h:
##########
@@ -140,6 +140,21 @@ class ICEBERG_EXPORT Catalog {
const std::string& location,
const std::unordered_map<std::string, std::string>& properties) = 0;
+ /// \brief Start a transaction to replace a table
+ ///
+ /// \param identifier a table identifier
+ /// \param schema a schema
+ /// \param spec a partition spec
+ /// \param location a location for the table; leave empty if unspecified
+ /// \param properties a string map of table properties
+ /// \param orCreate whether to create the table if not exists
+ /// \return a Transaction to replace the table or ErrorKind::kNotFound if the table
+ /// doesn't exist and orCreate is false
+ virtual Result<std::shared_ptr<Transaction>> StageReplaceTable(
+ const TableIdentifier& identifier, const Schema& schema, const PartitionSpec& spec,
+ const std::string& location,
+ const std::unordered_map<std::string, std::string>& properties, bool orCreate) = 0;
+
Review Comment:
```suggestion
```
##########
src/iceberg/catalog.h:
##########
@@ -223,6 +247,16 @@ class ICEBERG_EXPORT Catalog {
///
/// \return the Transaction to create the table
virtual std::unique_ptr<Transaction> StageCreate() = 0;
+
+ /// \brief Starts a transaction to replace the table
+ ///
+ /// \return the Transaction to replace the table
+ virtual std::unique_ptr<Transaction> StageReplace() = 0;
+
+ /// \brief Starts a transaction to create or replace the table
+ ///
+ /// \return the Transaction to create or replace the table
+ virtual std::unique_ptr<Transaction> StageCreateOrReplace() = 0;
Review Comment:
```suggestion
```
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: add rename table interface to catalog [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #281:
URL: https://github.com/apache/iceberg-cpp/pull/281#issuecomment-3519582877
Thanks @lishuxu for working on this and @shangxinli @HuaHuaY @HeartLinked for the review. Let's improve it in followup PRs and move forward!
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: add table create/replace/update interface to catalog [iceberg-cpp]
Posted by "lishuxu (via GitHub)" <gi...@apache.org>.
lishuxu commented on code in PR #281:
URL: https://github.com/apache/iceberg-cpp/pull/281#discussion_r2506883456
##########
src/iceberg/catalog.h:
##########
@@ -223,6 +247,16 @@ class ICEBERG_EXPORT Catalog {
///
/// \return the Transaction to create the table
virtual std::unique_ptr<Transaction> StageCreate() = 0;
+
+ /// \brief Starts a transaction to replace the table
+ ///
+ /// \return the Transaction to replace the table
+ virtual std::unique_ptr<Transaction> StageReplace() = 0;
+
+ /// \brief Starts a transaction to create or replace the table
+ ///
+ /// \breturn the Transaction to create or replace the table
Review Comment:
Good catch! Fixed the typo.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: add table create/replace/update interface to catalog [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #281:
URL: https://github.com/apache/iceberg-cpp/pull/281#discussion_r2512589312
##########
src/iceberg/transaction.h:
##########
@@ -42,12 +44,23 @@ class ICEBERG_EXPORT Transaction {
/// \return a new AppendFiles
virtual std::shared_ptr<AppendFiles> NewAppend() = 0;
+ /// \brief Apply multiple metadata updates to this transaction
+ ///
+ /// \param requirements the table requirements to validate
+ /// \param updates the table updates to apply
+ /// \return Status::OK if all updates were queued successfully
+ virtual Status UpdateTable(
Review Comment:
Why do we need this? I don't think `Transaction` is the entrance to create a single update action and `Catalog` is responsible of this. Let's remove it.
##########
src/iceberg/transaction.h:
##########
@@ -42,12 +44,23 @@ class ICEBERG_EXPORT Transaction {
/// \return a new AppendFiles
virtual std::shared_ptr<AppendFiles> NewAppend() = 0;
+ /// \brief Apply multiple metadata updates to this transaction
+ ///
+ /// \param requirements the table requirements to validate
+ /// \param updates the table updates to apply
+ /// \return Status::OK if all updates were queued successfully
+ virtual Status UpdateTable(
+ const std::vector<std::unique_ptr<TableRequirement>>& requirements,
+ const std::vector<std::unique_ptr<TableUpdate>>& updates) = 0;
+
Review Comment:
```suggestion
```
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: add rename table interface to catalog [iceberg-cpp]
Posted by "HuaHuaY (via GitHub)" <gi...@apache.org>.
HuaHuaY commented on code in PR #281:
URL: https://github.com/apache/iceberg-cpp/pull/281#discussion_r2513029035
##########
src/iceberg/catalog/memory/in_memory_catalog.cc:
##########
@@ -415,6 +415,12 @@ Status InMemoryCatalog::DropTable(const TableIdentifier& identifier, bool purge)
return root_namespace_->UnregisterTable(identifier);
}
+Status InMemoryCatalog::RenameTable(const TableIdentifier& from,
+ const TableIdentifier& to) {
+ std::unique_lock lock(mutex_);
Review Comment:
If we don't need to move `lock`, we can use `std::lock_guard<>` instead. Can you help to modify the other cases in this file? Or we can open a new PR to do this.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: add rename table interface to catalog [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac merged PR #281:
URL: https://github.com/apache/iceberg-cpp/pull/281
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: add table create/replace/update interface to catalog [iceberg-cpp]
Posted by "lishuxu (via GitHub)" <gi...@apache.org>.
lishuxu commented on code in PR #281:
URL: https://github.com/apache/iceberg-cpp/pull/281#discussion_r2506884093
##########
src/iceberg/table.h:
##########
@@ -109,6 +109,11 @@ class ICEBERG_EXPORT Table {
/// filter data.
virtual std::unique_ptr<TableScanBuilder> NewScan() const;
+ /// \brief Create a new transaction for this table
+ ///
+ /// \return a shared pointer to the new Transaction
+ virtual std::shared_ptr<Transaction> NewTransaction();
Review Comment:
Yes, it should be const.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: add rename table interface to catalog [iceberg-cpp]
Posted by "HeartLinked (via GitHub)" <gi...@apache.org>.
HeartLinked commented on code in PR #281:
URL: https://github.com/apache/iceberg-cpp/pull/281#discussion_r2513006747
##########
src/iceberg/transaction.h:
##########
@@ -21,8 +21,10 @@
#pragma once
#include <memory>
+#include <vector>
Review Comment:
Why include this?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: add table create/replace/update interface to catalog [iceberg-cpp]
Posted by "shangxinli (via GitHub)" <gi...@apache.org>.
shangxinli commented on code in PR #281:
URL: https://github.com/apache/iceberg-cpp/pull/281#discussion_r2484177969
##########
src/iceberg/catalog.h:
##########
@@ -223,6 +247,16 @@ class ICEBERG_EXPORT Catalog {
///
/// \return the Transaction to create the table
virtual std::unique_ptr<Transaction> StageCreate() = 0;
+
+ /// \brief Starts a transaction to replace the table
+ ///
+ /// \return the Transaction to replace the table
+ virtual std::unique_ptr<Transaction> StageReplace() = 0;
+
+ /// \brief Starts a transaction to create or replace the table
+ ///
+ /// \breturn the Transaction to create or replace the table
Review Comment:
typo? '\return'?
##########
src/iceberg/table.h:
##########
@@ -109,6 +109,11 @@ class ICEBERG_EXPORT Table {
/// filter data.
virtual std::unique_ptr<TableScanBuilder> NewScan() const;
+ /// \brief Create a new transaction for this table
+ ///
+ /// \return a shared pointer to the new Transaction
+ virtual std::shared_ptr<Transaction> NewTransaction();
Review Comment:
We don't want to add 'const' just like NewScan() and io()?
##########
src/iceberg/transaction.h:
##########
@@ -42,12 +44,23 @@ class ICEBERG_EXPORT Transaction {
/// \return a new AppendFiles
virtual std::shared_ptr<AppendFiles> NewAppend() = 0;
+ /// \brief Apply multiple metadata updates to this transaction
+ ///
+ /// \param requirements the table requirements to validate
+ /// \param updates the table updates to apply
+ /// \return Status::OK if all updates were queued successfully
+ virtual Status UpdateTable(
+ const std::vector<std::unique_ptr<TableRequirement>>& requirements,
+ std::vector<std::unique_ptr<TableUpdate>> updates) = 0;
+
/// \brief Apply the pending changes from all actions and commit
///
- /// May throw ValidationException if any update cannot be applied to the current table
- /// metadata. May throw CommitFailedException if the updates cannot be committed due to
- /// conflicts.
- virtual void CommitTransaction() = 0;
Review Comment:
Do we want to keep backward compatibility with deprecation e.g.
[[deprecated("Use CommitTransaction() that returns Status")]]
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: add table create/replace/update interface to catalog [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #281:
URL: https://github.com/apache/iceberg-cpp/pull/281#discussion_r2484858388
##########
src/iceberg/transaction.h:
##########
@@ -42,12 +44,23 @@ class ICEBERG_EXPORT Transaction {
/// \return a new AppendFiles
virtual std::shared_ptr<AppendFiles> NewAppend() = 0;
+ /// \brief Apply multiple metadata updates to this transaction
+ ///
+ /// \param requirements the table requirements to validate
+ /// \param updates the table updates to apply
+ /// \return Status::OK if all updates were queued successfully
+ virtual Status UpdateTable(
+ const std::vector<std::unique_ptr<TableRequirement>>& requirements,
+ std::vector<std::unique_ptr<TableUpdate>> updates) = 0;
+
/// \brief Apply the pending changes from all actions and commit
///
- /// May throw ValidationException if any update cannot be applied to the current table
- /// metadata. May throw CommitFailedException if the updates cannot be committed due to
- /// conflicts.
- virtual void CommitTransaction() = 0;
Review Comment:
I think we can break any existing API before 1.0.0. We just released 0.1.0 a while ago and I don't think it is ready for production yet.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat: add table create/replace/update interface to catalog [iceberg-cpp]
Posted by "lishuxu (via GitHub)" <gi...@apache.org>.
lishuxu commented on code in PR #281:
URL: https://github.com/apache/iceberg-cpp/pull/281#discussion_r2512514666
##########
src/iceberg/table.h:
##########
@@ -109,6 +109,11 @@ class ICEBERG_EXPORT Table {
/// filter data.
virtual std::unique_ptr<TableScanBuilder> NewScan() const;
+ /// \brief Create a new transaction for this table
+ ///
+ /// \return a shared pointer to the new Transaction
+ virtual std::shared_ptr<Transaction> NewTransaction() const;
Review Comment:
OK, that's would be better
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org