You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "twuebi (via GitHub)" <gi...@apache.org> on 2025/09/22 21:06:17 UTC
[PR] feat(metadata): snapshot validations [iceberg-go]
twuebi opened a new pull request, #575:
URL: https://github.com/apache/iceberg-go/pull/575
Adds snapshot validations & tests
--
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(metadata): snapshot validations [iceberg-go]
Posted by "twuebi (via GitHub)" <gi...@apache.org>.
twuebi commented on code in PR #575:
URL: https://github.com/apache/iceberg-go/pull/575#discussion_r2375139658
##########
table/metadata.go:
##########
@@ -1313,6 +1366,14 @@ func (c *commonMetadata) validate() error {
c.constructRefs()
+ if err := c.checkMainRefMatchesCurrentSnapshot(); err != nil {
+ return err
+ }
Review Comment:
Just checked pyiceberg, that may be a bug? Setting `main` to some snapshot and omitting `current-snapshot-id` is accepted but calling `metadata.current_snapshot()` gives `None` in that case, that'd be in violation of:
> long ID of the current table snapshot; must be the same as the current ID of the main branch in refs.
I read that as, if the main branch in refs exist, the current-snapshot-id must match, if the main branch in refs does not exist, the current-snapshot-id may be omitted.
--
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(metadata): snapshot validations [iceberg-go]
Posted by "twuebi (via GitHub)" <gi...@apache.org>.
twuebi commented on code in PR #575:
URL: https://github.com/apache/iceberg-go/pull/575#discussion_r2371896159
##########
table/metadata.go:
##########
@@ -1313,6 +1366,14 @@ func (c *commonMetadata) validate() error {
c.constructRefs()
+ if err := c.checkMainRefMatchesCurrentSnapshot(); err != nil {
+ return err
+ }
Review Comment:
So my take-away is, it's optional but only if there's no main-branch in `refs`.
--
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(metadata): snapshot validations [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade merged PR #575:
URL: https://github.com/apache/iceberg-go/pull/575
--
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(metadata): snapshot validations [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #575:
URL: https://github.com/apache/iceberg-go/pull/575#discussion_r2370310131
##########
table/metadata.go:
##########
@@ -380,6 +379,14 @@ func (b *MetadataBuilder) RemoveSnapshots(snapshotIds []int64) error {
b.snapshotLog = slices.DeleteFunc(b.snapshotLog, func(e SnapshotLogEntry) bool {
return slices.Contains(snapshotIds, e.SnapshotID)
})
+
+ newRefs := make(map[string]SnapshotRef)
+ for name, ref := range b.refs {
+ if _, err := b.SnapshotByID(ref.SnapshotID); err == nil {
+ newRefs[name] = ref
+ }
+ }
Review Comment:
why build this map? we just drop it and don't do anything with it
##########
table/metadata.go:
##########
@@ -354,11 +354,10 @@ func (b *MetadataBuilder) AddSnapshot(snapshot *Snapshot) error {
} else if s, _ := b.SnapshotByID(snapshot.SnapshotID); s != nil {
return fmt.Errorf("can't add snapshot with id %d, already exists", snapshot.SnapshotID)
} else if b.formatVersion == 2 &&
- snapshot.SequenceNumber > 0 &&
snapshot.ParentSnapshotID != nil &&
snapshot.SequenceNumber <= *b.lastSequenceNumber {
return fmt.Errorf("can't add snapshot with sequence number %d, must be > than last sequence number %d",
- snapshot.SequenceNumber, b.lastSequenceNumber)
+ snapshot.SequenceNumber, *b.lastSequenceNumber)
Review Comment:
are we sure this can't be nil here? Should we have a nil check?
##########
table/metadata.go:
##########
@@ -1313,6 +1366,14 @@ func (c *commonMetadata) validate() error {
c.constructRefs()
+ if err := c.checkMainRefMatchesCurrentSnapshot(); err != nil {
+ return err
+ }
Review Comment:
is this a requirement in the spec? That the `main` ref must be the current snapshot?
--
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(metadata): snapshot validations [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #575:
URL: https://github.com/apache/iceberg-go/pull/575#discussion_r2370382385
##########
table/metadata.go:
##########
@@ -354,11 +354,10 @@ func (b *MetadataBuilder) AddSnapshot(snapshot *Snapshot) error {
} else if s, _ := b.SnapshotByID(snapshot.SnapshotID); s != nil {
return fmt.Errorf("can't add snapshot with id %d, already exists", snapshot.SnapshotID)
} else if b.formatVersion == 2 &&
- snapshot.SequenceNumber > 0 &&
snapshot.ParentSnapshotID != nil &&
snapshot.SequenceNumber <= *b.lastSequenceNumber {
return fmt.Errorf("can't add snapshot with sequence number %d, must be > than last sequence number %d",
- snapshot.SequenceNumber, b.lastSequenceNumber)
+ snapshot.SequenceNumber, *b.lastSequenceNumber)
Review Comment:
If we know that we're on version 2 and that we should never be nil in version 2 then I'm okay with forgoing the nil check for now.
--
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(metadata): snapshot validations [iceberg-go]
Posted by "twuebi (via GitHub)" <gi...@apache.org>.
twuebi commented on code in PR #575:
URL: https://github.com/apache/iceberg-go/pull/575#discussion_r2371894614
##########
table/metadata.go:
##########
@@ -1313,6 +1366,14 @@ func (c *commonMetadata) validate() error {
c.constructRefs()
+ if err := c.checkMainRefMatchesCurrentSnapshot(); err != nil {
+ return err
+ }
Review Comment:
This is called from the TableMetadata constructor of java:
```java
private static Map<String, SnapshotRef> validateRefs(
Long currentSnapshotId,
Map<String, SnapshotRef> inputRefs,
Map<Long, Snapshot> snapshotsById) {
for (SnapshotRef ref : inputRefs.values()) {
Preconditions.checkArgument(
snapshotsById.containsKey(ref.snapshotId()),
"Snapshot for reference %s does not exist in the existing snapshots list",
ref);
}
SnapshotRef main = inputRefs.get(SnapshotRef.MAIN_BRANCH);
if (currentSnapshotId != -1) {
Preconditions.checkArgument(
main == null || currentSnapshotId == main.snapshotId(),
"Current snapshot ID does not match main branch (%s != %s)",
currentSnapshotId,
main != null ? main.snapshotId() : null);
} else {
Preconditions.checkArgument(
main == null, "Current snapshot is not set, but main branch exists: %s", main);
}
return inputRefs;
}
```
Then there's also this check at the end of the constructor:
```java
private void validateCurrentSnapshot() {
Preconditions.checkArgument(
currentSnapshotId < 0 || snapshotsById.containsKey(currentSnapshotId),
"Invalid table metadata: Cannot find current version");
}
```
Parsing a TableMetadata with a main branch but without current-snapshot-id:
```json
{
...
"snapshots": [
{
"snapshot-id": 3051729675574597004,
"timestamp-ms": 1515100955770,
"sequence-number": 0,
"summary": {
"operation": "append"
},
"manifest-list": "s3://a/b/1.avro"
},
{
"snapshot-id": 3055729675574597004,
"parent-snapshot-id": 3051729675574597004,
"timestamp-ms": 1555100955770,
"sequence-number": 1,
"summary": {
"operation": "append"
},
"manifest-list": "s3://a/b/2.avro",
"schema-id": 1
}
],
"refs": {"main": {"snapshot-id": 3051729675574597004, "type": "branch"}}
}
```
leads to this:
```
Current snapshot is not set, but main branch exists: SnapshotRef{snapshotId=3051729675574597004, type=BRANCH, minSnapshotsToKeep=null, maxSnapshotAgeMs=null, maxRefAgeMs=null}
java.lang.IllegalArgumentException: Current snapshot is not set, but main branch exists: SnapshotRef{snapshotId=3051729675574597004, type=BRANCH, minSnapshotsToKeep=null, maxSnapshotAgeMs=null, maxRefAgeMs=null}
at org.apache.iceberg.relocated.com.google.common.base.Preconditions.checkArgument(Preconditions.java:217)
at org.apache.iceberg.TableMetadata.validateRefs(TableMetadata.java:870)
at org.apache.iceberg.TableMetadata.<init>(TableMetadata.java:350)
at org.apache.iceberg.TableMetadataParser.fromJson(TableMetadataParser.java:583)
at org.apache.iceberg.TableMetadataParser.lambda$fromJson$0(TableMetadataParser.java:327)
at org.apache.iceberg.util.JsonUtil.parse(JsonUtil.java:104)
at org.apache.iceberg.TableMetadataParser.fromJson(TableMetadataParser.java:327)
at org.apache.iceberg.TableMetadataParser.fromJson(TableMetadataParser.java:316)
```
--
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(metadata): snapshot validations [iceberg-go]
Posted by "twuebi (via GitHub)" <gi...@apache.org>.
twuebi commented on code in PR #575:
URL: https://github.com/apache/iceberg-go/pull/575#discussion_r2370334645
##########
table/metadata.go:
##########
@@ -380,6 +379,14 @@ func (b *MetadataBuilder) RemoveSnapshots(snapshotIds []int64) error {
b.snapshotLog = slices.DeleteFunc(b.snapshotLog, func(e SnapshotLogEntry) bool {
return slices.Contains(snapshotIds, e.SnapshotID)
})
+
+ newRefs := make(map[string]SnapshotRef)
+ for name, ref := range b.refs {
+ if _, err := b.SnapshotByID(ref.SnapshotID); err == nil {
+ newRefs[name] = ref
+ }
+ }
Review Comment:
missed the assignment `b.refs = newRefs` when extracting this from my has-all-changes-branch, will add a test, this should've been caught by one..
--
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(metadata): snapshot validations [iceberg-go]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on code in PR #575:
URL: https://github.com/apache/iceberg-go/pull/575#discussion_r2379636309
##########
table/metadata.go:
##########
@@ -1313,6 +1366,14 @@ func (c *commonMetadata) validate() error {
c.constructRefs()
+ if err := c.checkMainRefMatchesCurrentSnapshot(); err != nil {
+ return err
+ }
Review Comment:
Reading the spec and from the info above (thx btw), im interpreting the `ref` to be a weak reference of the `current-snapshot-id`. The `current-snapshot-id` is the source of truth and the `main` branch just refers to whatever its set to
See this SetSnapshotOperation op:
https://github.com/apache/iceberg/blob/fbb7136fa9c0605ed2553053606276857c306f99/core/src/main/java/org/apache/iceberg/SetSnapshotOperation.java#L123
and the doc from `refs`
<img width="681" height="236" alt="Screenshot 2025-09-25 at 8 51 06 AM" src="https://github.com/user-attachments/assets/d518d03f-5a81-497e-b768-67e6438f8833" />
--
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(metadata): snapshot validations [iceberg-go]
Posted by "twuebi (via GitHub)" <gi...@apache.org>.
twuebi commented on code in PR #575:
URL: https://github.com/apache/iceberg-go/pull/575#discussion_r2370345563
##########
table/metadata.go:
##########
@@ -354,11 +354,10 @@ func (b *MetadataBuilder) AddSnapshot(snapshot *Snapshot) error {
} else if s, _ := b.SnapshotByID(snapshot.SnapshotID); s != nil {
return fmt.Errorf("can't add snapshot with id %d, already exists", snapshot.SnapshotID)
} else if b.formatVersion == 2 &&
- snapshot.SequenceNumber > 0 &&
snapshot.ParentSnapshotID != nil &&
snapshot.SequenceNumber <= *b.lastSequenceNumber {
return fmt.Errorf("can't add snapshot with sequence number %d, must be > than last sequence number %d",
- snapshot.SequenceNumber, b.lastSequenceNumber)
+ snapshot.SequenceNumber, *b.lastSequenceNumber)
Review Comment:
Shouldn't be nil since it can only be nil for version 1 and we check that we're on version 2 here, I think we validate the other cases, can add a sanity check
--
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(metadata): snapshot validations [iceberg-go]
Posted by "twuebi (via GitHub)" <gi...@apache.org>.
twuebi commented on code in PR #575:
URL: https://github.com/apache/iceberg-go/pull/575#discussion_r2370392104
##########
table/metadata.go:
##########
@@ -1313,6 +1366,14 @@ func (c *commonMetadata) validate() error {
c.constructRefs()
+ if err := c.checkMainRefMatchesCurrentSnapshot(); err != nil {
+ return err
+ }
Review Comment:
Will check tomorrow what other implementations (Java,rust) do, I think I got this from rust, maybe there was a discussion in the rust pr. But yeah by the spec it shouldn't be required.
--
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(metadata): snapshot validations [iceberg-go]
Posted by "twuebi (via GitHub)" <gi...@apache.org>.
twuebi commented on code in PR #575:
URL: https://github.com/apache/iceberg-go/pull/575#discussion_r2370350089
##########
table/metadata.go:
##########
@@ -354,11 +354,10 @@ func (b *MetadataBuilder) AddSnapshot(snapshot *Snapshot) error {
} else if s, _ := b.SnapshotByID(snapshot.SnapshotID); s != nil {
return fmt.Errorf("can't add snapshot with id %d, already exists", snapshot.SnapshotID)
} else if b.formatVersion == 2 &&
- snapshot.SequenceNumber > 0 &&
snapshot.ParentSnapshotID != nil &&
snapshot.SequenceNumber <= *b.lastSequenceNumber {
return fmt.Errorf("can't add snapshot with sequence number %d, must be > than last sequence number %d",
- snapshot.SequenceNumber, b.lastSequenceNumber)
+ snapshot.SequenceNumber, *b.lastSequenceNumber)
Review Comment:
Not sure what error to return tho, that'd be a bug in our implementation, I don't think returning an error in that situation is helpful? I think we may be in this situation? https://burntsushi.net/unwrap/#why-not-return-an-error-instead-of-panicking
--
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(metadata): snapshot validations [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #575:
URL: https://github.com/apache/iceberg-go/pull/575#discussion_r2370385992
##########
table/metadata.go:
##########
@@ -1313,6 +1366,14 @@ func (c *commonMetadata) validate() error {
c.constructRefs()
+ if err := c.checkMainRefMatchesCurrentSnapshot(); err != nil {
+ return err
+ }
Review Comment:
According to that image, the `current-snapshot-id` is optional, so we shouldn't error if it's nil and the main branch exists. We should just assume that the `main` branch is the current snapshot if this isn't set, right?
--
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(metadata): snapshot validations [iceberg-go]
Posted by "twuebi (via GitHub)" <gi...@apache.org>.
twuebi commented on code in PR #575:
URL: https://github.com/apache/iceberg-go/pull/575#discussion_r2370338605
##########
table/metadata.go:
##########
@@ -1313,6 +1366,14 @@ func (c *commonMetadata) validate() error {
c.constructRefs()
+ if err := c.checkMainRefMatchesCurrentSnapshot(); err != nil {
+ return err
+ }
Review Comment:
Yeah, part of the spec:
<img width="847" height="138" alt="image" src="https://github.com/user-attachments/assets/6357ab77-f314-456c-891f-c29994f9c3c3" />
--
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(metadata): snapshot validations [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #575:
URL: https://github.com/apache/iceberg-go/pull/575#discussion_r2372501106
##########
table/metadata.go:
##########
@@ -1313,6 +1366,14 @@ func (c *commonMetadata) validate() error {
c.constructRefs()
+ if err := c.checkMainRefMatchesCurrentSnapshot(); err != nil {
+ return err
+ }
Review Comment:
@Fokko @kevinjqliu is this something the spec be more explicit about? Or is this behavior divergent from the spec in java?
--
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(metadata): snapshot validations [iceberg-go]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on code in PR #575:
URL: https://github.com/apache/iceberg-go/pull/575#discussion_r2379638607
##########
table/metadata.go:
##########
@@ -1313,6 +1366,14 @@ func (c *commonMetadata) validate() error {
c.constructRefs()
+ if err := c.checkMainRefMatchesCurrentSnapshot(); err != nil {
+ return err
+ }
Review Comment:
> Just checked pyiceberg, that may be a bug? Setting main to some snapshot and omitting current-snapshot-id is accepted but calling metadata.current_snapshot() gives None in that case, that'd be in violation of:
we just implemented writing to branch, so this might be a bug :)
--
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