fqaiser94 commented on code in PR #6513:
URL: https://github.com/apache/iceberg/pull/6513#discussion_r1142718446
##########
api/src/main/java/org/apache/iceberg/PendingUpdate.java:
##########
@@ -29,6 +31,28 @@
*/
public interface PendingUpdate<T> {
+ /**
+ * Accepts a predicate which will be used to validate whether it is safe to
commit the pending
+ * changes to the current version of the table at commit time.
+ *
+ * <p>For example, this method could be used to ensure the pending changes
are only committed if a
+ * given snapshot property is present in the current version of the table.
+ *
+ * <p>This method can be called multiple times to add multiple predicates if
necessary.
+ *
+ * @param test The predicate which will be used to validate whether it is
safe to commit the
+ * pending changes to the current version of the table at commit time.
Any attempts to modify
+ * the table passed to the predicate will throw an exception as this
table is read-only.
+ * @param message The message that will be included in the {@link
ValidationException} that will
+ * be thrown if the test returns false.
+ * @param args The args that will be included in the {@link
ValidationException} that will be
+ * thrown if the test returns false.
+ */
+ @FormatMethod
+ default void validateCurrentTable(Predicate<Table> test, String message,
Object... args) {
Review Comment:
This is very close to what I originally proposed ;)
Although I would prefer if the `commitIf` method accepted a list of
validations rather than a single validation as that would allows users to
specify a unique message per failed predicate (which can be useful in
recovering from the specific `ValidationException`). To be concrete, this is
what I would reccommend:
```
public interface PendingUpdate<T> {
void commitIf(List<Validation> validations)
}
public class Validation {
private final Predicate<Table> predicate;
private final String message;
private final Object[] args;
@FormatMethod
Validation(Predicate<Table> predicate, String message, Object... args) {
this.predicate = predicate;
this.message = message;
this.args = args;
}
@SuppressWarnings("FormatStringAnnotation")
void check(Table currentTable) {
ValidationException.check(predicate.test(currentTable), message, args);
}
}
```
You can see a working implemention of this idea in one of my latest
(reverted) commits here:
https://github.com/apache/iceberg/pull/6513/commits/dd94de91beb28bdfa19128df54e92a929850f2f3
I think it's worth reconsidering in light of some of the recent
[comments](https://github.com/apache/iceberg/pull/6513#discussion_r1129872213)
as this could be a good compromise.
However @rdblue reccommended the current API design so I'd love to get his
thoughts before making this change.
--
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]
