Skip to content

Conversation

@feefs
Copy link

@feefs feefs commented Jan 3, 2026

I think this should be a follow-up to #9690 (set-partition-statistics and remove-partition-statistics were added to BaseUpdate's discriminator: YAML sequence but not TableUpdate's anyOf: YAML sequence) and #9240 (original refactor from enum: to discriminator:).

Without this, the JSON Schema I'm generating for my validator is incorrectly rejecting CommitTableRequest payloads with TableUpdate schema objects such as set-partition-statistics.


Ran my changes through:

make install
make generate
make lint

and everything passes.

- $ref: '#/components/schemas/SetPropertiesUpdate'
- $ref: '#/components/schemas/RemovePropertiesUpdate'
- $ref: '#/components/schemas/AddViewVersionUpdate'
- $ref: '#/components/schemas/SetCurrentViewVersionUpdate'
Copy link
Contributor

Choose a reason for hiding this comment

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

These two entries already exist in ViewUpdate. Why should we add them to TableUpdate?

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the catch, they shouldn't be copied over.

- $ref: '#/components/schemas/SetStatisticsUpdate'
- $ref: '#/components/schemas/RemoveStatisticsUpdate'
- $ref: '#/components/schemas/SetPartitionStatisticsUpdate'
- $ref: '#/components/schemas/RemovePartitionStatisticsUpdate'
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Author

@feefs feefs left a comment

Choose a reason for hiding this comment

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

Addressed comments.

@feefs feefs force-pushed the main branch 2 times, most recently from 8e1b014 to ef3a35f Compare January 9, 2026 21:12
@feefs
Copy link
Author

feefs commented Jan 9, 2026

@ebyhr friendly bump, I've rebased on top of the latest changes.

Copy link
Contributor

@ebyhr ebyhr left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@Fokko @singhpk234 @huaxingao Could you review this PR when you have time?

Copy link
Contributor

@singhpk234 singhpk234 left a comment

Choose a reason for hiding this comment

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

LGTM, i think its a miss mostly since java client supports it in ser-de, vote should not be required.
Lets see what others have to say !

@feefs
Copy link
Author

feefs commented Jan 17, 2026

@ebyhr @singhpk234 @huaxingao @Fokko another friendly bump, are the current approvals sufficient for merging?

@feefs feefs changed the title Open-API: backfill schemas from BaseUpdate to TableUpdate anyOf SPEC: backfill schemas from BaseUpdate to TableUpdate anyOf Jan 17, 2026
@feefs feefs force-pushed the main branch 2 times, most recently from 9310150 to 9aae4c3 Compare January 17, 2026 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants