Skip to content
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

Merged
merged 1 commit into from
Oct 20, 2023

Conversation

nastra
Copy link
Contributor

@nastra nastra commented Sep 29, 2023

No description provided.

@github-actions github-actions bot added the core label Sep 29, 2023
@nastra nastra force-pushed the only-ignore-operation-for-views branch from 37685f8 to 692efe1 Compare September 29, 2023 07:12
@nastra nastra force-pushed the only-ignore-operation-for-views branch from 3dec72b to 8ae63c2 Compare September 29, 2023 14:51
@nastra nastra requested a review from rdblue October 2, 2023 08:46
@rdblue
Copy link
Contributor

rdblue commented Oct 11, 2023

I think we may want to consider an alternative, which is to get rid of operation entirely. What is the value of operation? In a table, we use the operation to make assumptions about metadata without actually reading parts of the metadata tree. For views, I don't think there is a similar need -- we probably won't use this anywhere.

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.

@nastra nastra force-pushed the only-ignore-operation-for-views branch from 8ae63c2 to d688763 Compare October 13, 2023 07:07
@github-actions github-actions bot added API Specification Issues that may introduce spec changes. labels Oct 13, 2023
@nastra nastra force-pushed the only-ignore-operation-for-views branch from d688763 to a415ab9 Compare October 13, 2023 07:08
@nastra nastra changed the title Core: Include summary without 'operation' when comparing view versions Core: Derive 'operation' from view version Oct 13, 2023
@@ -161,7 +162,7 @@ private View create(ViewOperations ops) {
.defaultNamespace(defaultNamespace)
.defaultCatalog(defaultCatalog)
.timestampMillis(System.currentTimeMillis())
.putSummary("operation", "create")
.putAllSummary(EnvironmentContext.get())
Copy link
Contributor Author

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())
Copy link
Contributor Author

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")
Copy link
Contributor

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.

@rdblue rdblue merged commit 6f15175 into apache:main Oct 20, 2023
46 checks passed
@rdblue
Copy link
Contributor

rdblue commented Oct 20, 2023

Thanks, @nastra!

@nastra nastra deleted the only-ignore-operation-for-views branch October 21, 2023 07:24
@nastra nastra self-assigned this Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API core Specification Issues that may introduce spec changes.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants