rdblue commented on code in PR #14509:
URL: https://github.com/apache/iceberg/pull/14509#discussion_r2496295855
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -459,6 +467,23 @@ public void commit() {
return;
}
+ // Pass the snapshot ancestry of the base and the updated
snapshots to the
+ // validator.
+ String validationBranch =
Review Comment:
I [mentioned
this](https://github.com/apache/iceberg/pull/14510/files#r2496280751) on the
other PR because I wanted to look at how this is used: I think that this should
be called from within `validate`, which is the existing way that we add
validations to operations, or at least at the same time. The `validate` method
is overridden by children so we may not want to call this only in the parent,
but I think it makes sense to perform validation at the same time in all cases
(from the `apply` method) and using roughly the same data. That `validate` is
called with the current `TableMetadata` from refreshing the table and the
current snapshot of the target branch.
Another question is whether the "updated" history needs to be passed and I
think that the answer is no. If you're passing a callback to validate metadata,
I think that you want to make assertions about the committed table state.
Anything that you would get from the uncommitted snapshot from the current
operation can be embedded in the validation's closure. Since that's exactly
what is done in the other PR, I think that it is probably reasonable to
simplify this and pass only the current snapshot ancestry.
--
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]
