Skip to content

Core: Ensure reactivated view version uses correct timestamp #12821

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
nastra merged 3 commits into from
Apr 25, 2025
Merged

Core: Ensure reactivated view version uses correct timestamp #12821

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
27af919
Core: Ensure reactivated view version uses correct timestamp
lliangyu-lin Apr 17, 2025
c530c02
Update version added logic for history entry & fix unit tests
lliangyu-lin Apr 17, 2025
262c6b0
Update comment & re-trigger CI
lliangyu-lin Apr 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion core/src/main/java/org/apache/iceberg/view/ViewMetadata.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,9 +249,23 @@ public Builder setCurrentVersionId(int newVersionId) {
changes.add(new MetadataUpdate.SetCurrentViewVersion(newVersionId));
}

// Use the timestamp from the view version if it was added in current set of changes;

@nastra nastra Apr 17, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would slightly update/simplify this to

       // Use the timestamp from the view version if it was added in current set of changes;
       // otherwise, use the current system time. This handles cases where the view version
-      // was created in a past commit and is being re-activated.
+      // was set as current in the past and is being re-activated.
       boolean versionAddedInThisChange =
-          changes.stream()
-              .anyMatch(
-                  change ->
-                      change instanceof MetadataUpdate.AddViewVersion
-                          && ((MetadataUpdate.AddViewVersion) change).viewVersion().versionId()
-                              == newVersionId);
-
-      long timestamp =
-          versionAddedInThisChange ? version.timestampMillis() : System.currentTimeMillis();
+          changes(MetadataUpdate.AddViewVersion.class)
+              .anyMatch(added -> added.viewVersion().versionId() == newVersionId);

       this.historyEntry =
           ImmutableViewHistoryEntry.builder()
-              .timestampMillis(timestamp)
+              .timestampMillis(
+                  versionAddedInThisChange ? version.timestampMillis() : System.currentTimeMillis())
               .versionId(version.versionId())
               .build();

@lliangyu-lin lliangyu-lin Apr 17, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nastra Thanks for the review! I'll apply the comments and raise a revision.
But some concerns I have right now is that the change breaks existing unit tests like

iceberg/core/src/test/java/org/apache/iceberg/view/TestViewMetadataParser.java

Lines 218 to 239 in 9fc49e1

ViewMetadata expectedViewMetadata =
ViewMetadata.buildFrom(
ViewMetadata.buildFrom(
ViewMetadata.builder()
.assignUUID("fa6506c3-7681-40c8-86dc-e36561f83385")
.addSchema(TEST_SCHEMA)
.addVersion(version1)
.addVersion(version2)
.setLocation("s3://bucket/test/location")
.setProperties(
ImmutableMap.of(
"some-key",
"some-value",
ViewProperties.COMMENT,
"some-comment"))
.setCurrentVersionId(1)
.upgradeFormatVersion(1)
.build())
.setCurrentVersionId(2)
.build())
.setMetadataLocation(metadataLocation)
.build();

In .setCurrentVersionId(2), it's in different builder and have no local changes, causing the activation of view version to set to current timestamp even though version 2 never set as current before. And in the comment, we are trying to say only view version was set as current in the past will use current ts.

@nastra nastra Apr 25, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changing the unit test with the changes you have here in order to match the version-log that we compare against (we compare against a fixed timestamp defined in ValidViewMetadata.json) makes sense to me as this is a consequence of using the current time millis instead of the view version's time when setting the view version from 1 to 2 without actually adding view version 2 in the same builder.

// otherwise, use the current system time. This handles cases where the view version
// was created in a past commit and is being re-activated.
boolean versionAddedInThisChange =
changes.stream()
.anyMatch(
change ->
change instanceof MetadataUpdate.AddViewVersion
&& ((MetadataUpdate.AddViewVersion) change).viewVersion().versionId()
== newVersionId);

long timestamp =
versionAddedInThisChange ? version.timestampMillis() : System.currentTimeMillis();

this.historyEntry =
ImmutableViewHistoryEntry.builder()
.timestampMillis(version.timestampMillis())
.timestampMillis(timestamp)
.versionId(version.versionId())
.build();

Expand Down
42 changes: 42 additions & 0 deletions core/src/test/java/org/apache/iceberg/view/TestViewMetadata.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import java.util.List;
import java.util.Map;
import java.util.UUID;
import java.util.stream.Collectors;
import org.apache.iceberg.MetadataUpdate;
import org.apache.iceberg.Schema;
import org.apache.iceberg.catalog.Namespace;
Expand Down Expand Up @@ -396,6 +397,47 @@ public void viewVersionHistoryIsCorrectlyRetained() {
.hasMessage("Cannot set current version to unknown version: 1");
}

@Test
public void versionHistoryEntryMaintainCorrectTimeline() {
ViewVersion viewVersionOne = newViewVersion(1, "select * from ns.tbl");
ViewVersion viewVersionTwo = newViewVersion(2, "select count(*) from ns.tbl");
ViewVersion viewVersionThree = newViewVersion(3, "select count(*) as count from ns.tbl");

long ts1 = viewVersionOne.timestampMillis();
long ts2 = viewVersionTwo.timestampMillis();
long ts3 = viewVersionThree.timestampMillis();

// Create initial view metadata
ViewMetadata viewMetadata =
ViewMetadata.builder()
.setLocation("location")
.addSchema(new Schema(Types.NestedField.required(1, "x", Types.LongType.get())))
.addVersion(viewVersionOne)
.setCurrentVersionId(1)
.build();

// Update view metadata with multiple view versions in the same builder
ViewMetadata updated =
Comment thread
lliangyu-lin marked this conversation as resolved.
Outdated
ViewMetadata.buildFrom(viewMetadata)
.addVersion(viewVersionTwo)
.addVersion(viewVersionThree)
.setCurrentVersionId(3)
.build();

// Reactivate an old view version as current version
ViewMetadata reactiveOldView = ViewMetadata.buildFrom(updated).setCurrentVersionId(1).build();
List<ViewHistoryEntry> history = reactiveOldView.history();

List<Integer> versionIds =
history.stream().map(ViewHistoryEntry::versionId).collect(Collectors.toList());
assertThat(versionIds).containsExactly(1, 3, 1);

// Verify timestamps in the history entries
assertThat(history.get(0).timestampMillis()).isEqualTo(ts1).isLessThanOrEqualTo(ts2);
assertThat(history.get(1).timestampMillis()).isEqualTo(ts3).isGreaterThanOrEqualTo(ts2);
assertThat(history.get(2).timestampMillis()).isGreaterThanOrEqualTo(ts3);
}

@Test
public void versionsAddedInCurrentBuildAreRetained() {
ViewVersion v1 = newViewVersion(1, "select 1 as count");
Expand Down