-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Core: Derive 'operation' from view version #8678
Conversation
37685f8
to
692efe1
Compare
core/src/test/java/org/apache/iceberg/view/TestViewMetadata.java
Outdated
Show resolved
Hide resolved
3dec72b
to
8ae63c2
Compare
I think we may want to consider an alternative, which is to get rid of If we don't need operation to make something faster, then why include it at all? It just creates an awkward situation here where we need special logic to deduplicate a version. Getting rid of it and checking the whole summary makes more sense to me. |
8ae63c2
to
d688763
Compare
d688763
to
a415ab9
Compare
@@ -161,7 +162,7 @@ private View create(ViewOperations ops) { | |||
.defaultNamespace(defaultNamespace) | |||
.defaultCatalog(defaultCatalog) | |||
.timestampMillis(System.currentTimeMillis()) | |||
.putSummary("operation", "create") | |||
.putAllSummary(EnvironmentContext.get()) |
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.
in https://iceberg.apache.org/view-spec/#summary we mention that the summary might contain the engine name/version, so I figured it makes sense to add this info from EnvironmentContext
here
@@ -99,7 +100,7 @@ public void basicCreateView() { | |||
.timestampMillis(view.currentVersion().timestampMillis()) | |||
.versionId(1) | |||
.schemaId(0) | |||
.putSummary("operation", "create") | |||
.summary(view.currentVersion().summary()) |
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.
we could also use EnvironmentContext.get()
here for comparison, but I'm not sure if this might later cause test issues when view support is added for other engines
@@ -335,7 +335,7 @@ public void metadataCompression(String fileName) throws IOException { | |||
.schemaId(0) | |||
.versionId(1) | |||
.timestampMillis(23L) | |||
.putSummary("operation", "create") | |||
.putSummary("user", "some-user") |
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.
It's probably best to use a fake property here rather than making someone think that user
is a defined property or would matter. This is very minor though.
Thanks, @nastra! |
No description provided.