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

Add missing last-column-id to spec #6701

Closed
wants to merge 1 commit into from
Closed

Conversation

snazy
Copy link
Member

@snazy snazy commented Jan 30, 2023

No description provided.

@@ -1541,6 +1541,8 @@ components:
properties:
snapshot:
$ref: '#/components/schemas/Snapshot'
last-column-id:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be added to to AddSchemaUpdate instead of AddSnapshotUpdate

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, not to AddSnapshotUpdate.

@rdblue
Copy link
Contributor

rdblue commented Jan 31, 2023

The reason why this wasn't in the original spec is because we can technically update it by getting the highest assigned ID from the new schema. Because it didn't strictly need to be part of the spec here, we omitted it. Still, we send this in the actual implementation because it simplifies the server side.

I'm ambivalent about whether we actually should include this in the spec. Keeping the spec simple is usually the best option. But I can see the argument for doing this. Maybe we should update the test service to no longer require the last-column-id.

FYI @bryanck.

@snazy
Copy link
Member Author

snazy commented Feb 2, 2023

(Side note: I no longer use the the rest-spec in my experiments - for various reasons - and use the JSON serialization from iceberg-core.)

But IMO the spec should exactly reflect the behavior (or better. vice versa). Otherwise, what's the point of having a specification, if the actual behavior is different. Users that do not use the JSON code in iceberg-core or the provided Python client will rely on the OpenAPI spec, potentially to generate curl requests or even generate their own client code from that spec.

@Fokko
Copy link
Contributor

Fokko commented Jun 5, 2023

@snazy Thanks for this PR, and I couldn't agree more that we need to keep the spec as close to the implementation as possible. I'm going to close this one now since #7445 has been merged.

@Fokko Fokko closed this Jun 5, 2023
@snazy snazy deleted the missing-lci branch June 7, 2023 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants