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: [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]
