You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "shangxinli (via GitHub)" <gi...@apache.org> on 2025/11/04 16:28:00 UTC
[PR] feat: implement validation for table update requirements [iceberg-cpp]
shangxinli opened a new pull request, #294:
URL: https://github.com/apache/iceberg-cpp/pull/294
Implement Validate() methods for remaining table requirement classes:
- AssertDoesNotExist: validates table doesn't exist
- AssertRefSnapshotID: validates snapshot references (branches/tags)
- AssertLastAssignedFieldId: validates last assigned field ID
- AssertLastAssignedPartitionId: validates last assigned partition ID
- AssertDefaultSpecID: validates default partition spec ID
- AssertDefaultSortOrderID: validates default sort order ID
All implementations follow the pattern established in AssertCurrentSchemaID and provide descriptive error messages for validation failures.
--
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: implement validation for table update requirements [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac merged PR #294:
URL: https://github.com/apache/iceberg-cpp/pull/294
--
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: implement validation for table update requirements [iceberg-cpp]
Posted by "shangxinli (via GitHub)" <gi...@apache.org>.
shangxinli commented on code in PR #294:
URL: https://github.com/apache/iceberg-cpp/pull/294#discussion_r2511541980
##########
src/iceberg/table_requirement.cc:
##########
@@ -45,12 +52,55 @@ Status AssertUUID::Validate(const TableMetadata* base) const {
}
Status AssertRefSnapshotID::Validate(const TableMetadata* base) const {
- return NotImplemented("AssertTableRefSnapshotID::Validate not implemented");
+ // Validate that a reference (branch or tag) points to the expected snapshot ID
+
+ if (base == nullptr) {
+ return CommitFailed("Requirement failed: current table metadata is missing");
+ }
+
+ auto it = base->refs.find(ref_name_);
+
+ // If snapshot_id is nullopt, the reference should not exist
+ if (!snapshot_id_.has_value()) {
+ if (it != base->refs.end()) {
+ return CommitFailed(
+ "Requirement failed: reference '{}' should not exist but found snapshot ID {}",
+ ref_name_, it->second->snapshot_id);
+ }
+ return {};
+ }
+
+ // snapshot_id has a value, so the reference should exist and match
+ if (it == base->refs.end()) {
+ return CommitFailed("Requirement failed: reference '{}' is missing in table metadata",
+ ref_name_);
+ }
+
+ if (it->second->snapshot_id != snapshot_id_.value()) {
+ return CommitFailed(
+ "Requirement failed: reference '{}' snapshot ID does not match (expected={}, "
+ "actual={})",
+ ref_name_, snapshot_id_.value(), it->second->snapshot_id);
+ }
+
+ return {};
}
Status AssertLastAssignedFieldId::Validate(const TableMetadata* base) const {
- return NotImplemented(
- "AssertCurrentTableLastAssignedFieldId::Validate not implemented");
+ // Validate that the last assigned field ID matches the expected value
+
+ if (base == nullptr) {
+ return CommitFailed("Requirement failed: current table metadata is missing");
+ }
+
+ if (base->last_column_id != last_assigned_field_id_) {
Review Comment:
Good catch
##########
src/iceberg/table_requirement.cc:
##########
@@ -75,16 +125,53 @@ Status AssertCurrentSchemaID::Validate(const TableMetadata* base) const {
}
Status AssertLastAssignedPartitionId::Validate(const TableMetadata* base) const {
- return NotImplemented(
- "AssertCurrentTableLastAssignedPartitionId::Validate not implemented");
+ // Validate that the last assigned partition ID matches the expected value
+
+ if (base == nullptr) {
+ return CommitFailed("Requirement failed: current table metadata is missing");
+ }
+
+ if (base->last_partition_id != last_assigned_partition_id_) {
Review Comment:
changed
--
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: implement validation for table update requirements [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #294:
URL: https://github.com/apache/iceberg-cpp/pull/294#discussion_r2513164254
##########
src/iceberg/table_requirement.cc:
##########
@@ -19,43 +19,77 @@
#include "iceberg/table_requirement.h"
+#include "iceberg/snapshot.h"
#include "iceberg/table_metadata.h"
+#include "iceberg/util/macros.h"
#include "iceberg/util/string_util.h"
namespace iceberg::table {
Status AssertDoesNotExist::Validate(const TableMetadata* base) const {
- return NotImplemented("AssertDoesNotExist::Validate not implemented");
+ if (base != nullptr) {
+ return CommitFailed("Requirement failed: table already exists");
+ }
+
+ return {};
}
Status AssertUUID::Validate(const TableMetadata* base) const {
- // Validate that the table UUID matches the expected value
-
if (base == nullptr) {
return CommitFailed("Requirement failed: current table metadata is missing");
}
if (!StringUtils::EqualsIgnoreCase(base->table_uuid, uuid_)) {
return CommitFailed(
- "Requirement failed: table UUID does not match (expected='{}', actual='{}')",
- uuid_, base->table_uuid);
+ "Requirement failed: table UUID does not match, expected {} != {}", uuid_,
+ base->table_uuid);
}
return {};
}
Status AssertRefSnapshotID::Validate(const TableMetadata* base) const {
- return NotImplemented("AssertTableRefSnapshotID::Validate not implemented");
+ if (base == nullptr) {
+ return CommitFailed("Requirement failed: current table metadata is missing");
+ }
+
+ if (auto ref = base->refs.find(ref_name_); ref != base->refs.cend()) {
+ const auto& snapshot_ref = ref->second;
+ ICEBERG_DCHECK(snapshot_ref != nullptr, "Snapshot reference is null");
+ std::string_view type =
+ snapshot_ref->type() == SnapshotRefType::kBranch ? "branch" : "tag";
+ if (!snapshot_id_.has_value()) {
+ // A null snapshot ID means the ref should not exist already
+ return CommitFailed("Requirement failed: {} '{}' was created concurrently", type,
+ ref_name_);
+ } else if (snapshot_id_.value() != snapshot_ref->snapshot_id) {
+ return CommitFailed("Requirement failed: {} '{}' has changed: expected id {} != {}",
+ type, ref_name_, snapshot_id_.value(),
+ snapshot_ref->snapshot_id);
+ }
+ } else if (snapshot_id_.has_value()) {
+ return CommitFailed("Requirement failed: branch or tag '{}' is missing, expected {}",
+ ref_name_, snapshot_id_.value());
+ }
+
+ return {};
}
Status AssertLastAssignedFieldId::Validate(const TableMetadata* base) const {
- return NotImplemented(
- "AssertCurrentTableLastAssignedFieldId::Validate not implemented");
+ if (base == nullptr) {
+ return CommitFailed("Requirement failed: current table metadata is missing");
+ }
+
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: implement validation for table update requirements [iceberg-cpp]
Posted by "shangxinli (via GitHub)" <gi...@apache.org>.
shangxinli commented on code in PR #294:
URL: https://github.com/apache/iceberg-cpp/pull/294#discussion_r2511542652
##########
src/iceberg/table_requirement.cc:
##########
@@ -45,12 +52,55 @@ Status AssertUUID::Validate(const TableMetadata* base) const {
}
Status AssertRefSnapshotID::Validate(const TableMetadata* base) const {
- return NotImplemented("AssertTableRefSnapshotID::Validate not implemented");
+ // Validate that a reference (branch or tag) points to the expected snapshot ID
+
+ if (base == nullptr) {
Review Comment:
Changed
--
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: implement validation for table update requirements [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #294:
URL: https://github.com/apache/iceberg-cpp/pull/294#discussion_r2501551193
##########
src/iceberg/table_requirement.cc:
##########
@@ -75,16 +125,53 @@ Status AssertCurrentSchemaID::Validate(const TableMetadata* base) const {
}
Status AssertLastAssignedPartitionId::Validate(const TableMetadata* base) const {
- return NotImplemented(
- "AssertCurrentTableLastAssignedPartitionId::Validate not implemented");
+ // Validate that the last assigned partition ID matches the expected value
+
+ if (base == nullptr) {
+ return CommitFailed("Requirement failed: current table metadata is missing");
+ }
+
+ if (base->last_partition_id != last_assigned_partition_id_) {
Review Comment:
```suggestion
if (base && base->last_partition_id != last_assigned_partition_id_) {
```
##########
src/iceberg/table_requirement.cc:
##########
@@ -45,12 +52,55 @@ Status AssertUUID::Validate(const TableMetadata* base) const {
}
Status AssertRefSnapshotID::Validate(const TableMetadata* base) const {
- return NotImplemented("AssertTableRefSnapshotID::Validate not implemented");
+ // Validate that a reference (branch or tag) points to the expected snapshot ID
+
+ if (base == nullptr) {
Review Comment:
This is inconsistent with the Java impl. Could you double check?
```java
public void validate(TableMetadata base) {
SnapshotRef ref = base.ref(name);
if (ref != null) {
String type = ref.isBranch() ? "branch" : "tag";
if (snapshotId == null) {
// a null snapshot ID means the ref should not exist already
throw new CommitFailedException(
"Requirement failed: %s %s was created concurrently", type, name);
} else if (snapshotId != ref.snapshotId()) {
throw new CommitFailedException(
"Requirement failed: %s %s has changed: expected id %s != %s",
type, name, snapshotId, ref.snapshotId());
}
} else if (snapshotId != null) {
throw new CommitFailedException(
"Requirement failed: branch or tag %s is missing, expected %s", name, snapshotId);
}
}
```
##########
src/iceberg/table_requirement.cc:
##########
@@ -45,12 +52,55 @@ Status AssertUUID::Validate(const TableMetadata* base) const {
}
Status AssertRefSnapshotID::Validate(const TableMetadata* base) const {
- return NotImplemented("AssertTableRefSnapshotID::Validate not implemented");
+ // Validate that a reference (branch or tag) points to the expected snapshot ID
+
+ if (base == nullptr) {
+ return CommitFailed("Requirement failed: current table metadata is missing");
+ }
+
+ auto it = base->refs.find(ref_name_);
+
+ // If snapshot_id is nullopt, the reference should not exist
+ if (!snapshot_id_.has_value()) {
+ if (it != base->refs.end()) {
+ return CommitFailed(
+ "Requirement failed: reference '{}' should not exist but found snapshot ID {}",
+ ref_name_, it->second->snapshot_id);
+ }
+ return {};
+ }
+
+ // snapshot_id has a value, so the reference should exist and match
+ if (it == base->refs.end()) {
+ return CommitFailed("Requirement failed: reference '{}' is missing in table metadata",
+ ref_name_);
+ }
+
+ if (it->second->snapshot_id != snapshot_id_.value()) {
+ return CommitFailed(
+ "Requirement failed: reference '{}' snapshot ID does not match (expected={}, "
+ "actual={})",
+ ref_name_, snapshot_id_.value(), it->second->snapshot_id);
+ }
+
+ return {};
}
Status AssertLastAssignedFieldId::Validate(const TableMetadata* base) const {
- return NotImplemented(
- "AssertCurrentTableLastAssignedFieldId::Validate not implemented");
+ // Validate that the last assigned field ID matches the expected value
+
+ if (base == nullptr) {
+ return CommitFailed("Requirement failed: current table metadata is missing");
+ }
+
+ if (base->last_column_id != last_assigned_field_id_) {
Review Comment:
```suggestion
if (base && base->last_column_id != last_assigned_field_id_) {
```
The Java impl allows base metadata is missing. I think it makes sense for a new table.
--
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