-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
OpenAPI: Add RemoveSchemas REST update type #12022
base: main
Are you sure you want to change the base?
Conversation
Similarly to removing unused specs, I started working on removing unused schemas. This PR is similar to #10846 |
637f0bf
to
89d177f
Compare
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.
@gaborkaszab I agree with this spec change, I think it's very important to maintaining table metadata sizes for cases where there's frequent schema evolution but we'll need to put this to a discuss/vote thread.
Could you send an email on the dev list? https://lists.apache.org/thread/2lo59wkhn8cm29wmokfmg2f18934qnln as an example
Thanks for taking a look, @amogh-jahagirdar ! |
cc @advancedxy |
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.
Thanks for pinging me. I think this is valuable especially for wide tables which have frequent schema update.
BTW, I think we should start the discuss/vote to get REST spec change passed first.
open-api/rest-catalog-open-api.py
Outdated
@@ -390,6 +390,11 @@ class RemovePartitionSpecsUpdate(BaseUpdate): | |||
spec_ids: List[int] = Field(..., alias='spec-ids') | |||
|
|||
|
|||
class RemoveSchemasUpdate(BaseUpdate): | |||
action: Optional[Literal['remove-schemas']] = None |
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.
why this is optional? Looks like all other actions are constant field.
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.
Good catch! There have been an OpenAPI version bump since I uploaded this PR and changed the type of these updates from enum to const. As a result the generated py file no longer has the action as optional.
89d177f
to
b9d6d32
Compare
allOf: | ||
- $ref: '#/components/schemas/BaseUpdate' | ||
required: | ||
- schema-ids |
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.
Is there a set of updated schema ids or a set of removed schema ids? I guess the later. Just to confirm.
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.
Set of schemas to remove from the table by ID, I think.
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.
Yes this is a set of schemas to remove by ID.
allOf: | ||
- $ref: '#/components/schemas/BaseUpdate' | ||
required: | ||
- schema-ids |
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.
Yes this is a set of schemas to remove by ID.
No description provided.