rdblue commented on code in PR #9725:
URL: https://github.com/apache/iceberg/pull/9725#discussion_r1490242520
##########
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));
Review Comment:
@nastra, I don't think there is a any order required for `versions`. Clients
could easily add new versions to the front or the back of the JSON list. That's
why this used a heuristic and assumed that greater version IDs were newer
versions.
I don't think there is any need to change how expiration works here. It's
not great that we use IDs, but I think it is better than the updated version,
which relies on undocumented ordering in versions. If we want to "fix" this,
then I think we should use more reliable information to order the versions. The
most reasonable choice is `version.timestampMillis()` descending.
--
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
