-
Notifications
You must be signed in to change notification settings - Fork 3k
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.
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
Conversation
| changes.add(new MetadataUpdate.SetCurrentViewVersion(newVersionId)); | ||
| } | ||
|
|
||
| // Use the timestamp from the view version if it was added in current set of changes; |
There was a problem hiding this comment.
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();
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
core/src/test/java/org/apache/iceberg/view/TestViewMetadata.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Eduard Tudenhoefner <[email protected]>
nastra
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks @lliangyu-lin
Description