amogh-jahagirdar commented on code in PR #9725:
URL: https://github.com/apache/iceberg/pull/9725#discussion_r1490117508
##########
core/src/main/java/org/apache/iceberg/view/ViewMetadata.java:
##########
@@ -438,6 +439,10 @@ public ViewMetadata build() {
metadataLocation == null || changes.isEmpty(),
"Cannot create view metadata with a metadata location and changes");
+ if (null != historyEntry) {
+ history.add(historyEntry);
+ }
Review Comment:
Shouldn't the history entry be non-null if we're at this point?
##########
core/src/main/java/org/apache/iceberg/view/ViewMetadata.java:
##########
@@ -479,21 +488,21 @@ public ViewMetadata build() {
metadataLocation);
}
- static List<ViewVersion> expireVersions(
- Map<Integer, ViewVersion> versionsById, int numVersionsToKeep) {
- // version ids are assigned sequentially. keep the latest versions by ID.
- List<Integer> ids = Lists.newArrayList(versionsById.keySet());
- ids.sort(Comparator.reverseOrder());
+ @VisibleForTesting
+ static List<ViewVersion> expireVersions(List<ViewVersion> versions, int
numVersionsToKeep) {
+ List<ViewVersion> versionsToKeep = Lists.newArrayList(versions);
- List<ViewVersion> retainedVersions = Lists.newArrayList();
- for (int idToKeep : ids.subList(0, numVersionsToKeep)) {
- retainedVersions.add(versionsById.get(idToKeep));
+ // version ids are assigned sequentially. keep the latest
numVersionsToKeep
+ if (numVersionsToKeep > 0 && versionsToKeep.size() > numVersionsToKeep) {
Review Comment:
If we want to be defensive since it's exposed within the package, I'd just
put a precondition check to fail if it's negative.
##########
core/src/main/java/org/apache/iceberg/view/ViewMetadata.java:
##########
@@ -438,6 +439,10 @@ public ViewMetadata build() {
metadataLocation == null || changes.isEmpty(),
"Cannot create view metadata with a metadata location and changes");
+ if (null != historyEntry) {
+ history.add(historyEntry);
+ }
Review Comment:
Oh nvm, from a ViewMetadata perspective it's not necessary that there's a
new history entry (properties update comes to mind).
--
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-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
