Open-API: Refactor TableRequirements by Fokko · Pull Request #7710 · apache/iceberg

Fokko · 2023-05-26T09:19:38Z

Noticed this when working on #6323

I believe the requirements are not defined in the right way. After generating code I got:

class TableRequirement(BaseModel):
    type: Literal[
        'assert-create',
        'assert-table-uuid',
        'assert-ref-snapshot-id',
        'assert-last-assigned-field-id',
        'assert-current-schema-id',
        'assert-last-assigned-partition-id',
        'assert-default-spec-id',
        'assert-default-sort-order-id',
    ]
    ref: Optional[str] = None
    uuid: Optional[str] = None
    snapshot_id: Optional[int] = Field(None, alias='snapshot-id')
    last_assigned_field_id: Optional[int] = Field(None, alias='last-assigned-field-id')
    current_schema_id: Optional[int] = Field(None, alias='current-schema-id')
    last_assigned_partition_id: Optional[int] = Field(
        None, alias='last-assigned-partition-id'
    )
    default_spec_id: Optional[int] = Field(None, alias='default-spec-id')
    default_sort_order_id: Optional[int] = Field(None, alias='default-sort-order-id')

Which encapsulates all the requirements. After the refactor in this PR, we'll end up with:

class TableRequirement(BaseModel):
    type: str


class AssertCreate(TableRequirement):
    """
    The table must not already exist; used for create transactions
    """

    type: Literal['assert-create']


class AssertTableUUID(TableRequirement):
    """
    The table UUID must match the requirement's `uuid`
    """

    type: Literal['assert-table-uuid']
    uuid: str


class AssertRefSnapshotId(TableRequirement):
    """
    The table branch or tag identified by the requirement's `ref` must reference the requirement's `snapshot-id`; if `snapshot-id` is `null` or missing, the ref must not already exist
    """

    type: Literal['assert-ref-snapshot-id']
    ref: str
    snapshot_id: int = Field(..., alias='snapshot-id')


class AssertLastAssignedFieldId(TableRequirement):
    """
    The table's last assigned column id must match the requirement's `last-assigned-field-id`
    """

    type: Literal['assert-last-assigned-field-id']
    last_assigned_field_id: int = Field(..., alias='last-assigned-field-id')


class AssertCurrentSchemaId(TableRequirement):
    """
    The table's current schema id must match the requirement's `current-schema-id`
    """

    type: Literal['assert-current-schema-id']
    current_schema_id: int = Field(..., alias='current-schema-id')


class AssertLastAssignedPartitionId(TableRequirement):
    """
    The table's last assigned partition id must match the requirement's `last-assigned-partition-id`
    """

    type: Literal['assert-last-assigned-partition-id']
    last_assigned_partition_id: int = Field(..., alias='last-assigned-partition-id')


class AssertDefaultSpecId(TableRequirement):
    """
    The table's default spec id must match the requirement's `default-spec-id`
    """

    type: Literal['assert-default-spec-id']
    default_spec_id: int = Field(..., alias='default-spec-id')


class AssertDefaultSortOrderId(TableRequirement):
    """
    The table's default sort order id must match the requirement's `default-sort-order-id`
    """

    type: Literal['assert-default-sort-order-id']
    default_sort_order_id: int = Field(..., alias='default-sort-order-id')

Which makes sense to me.

See below for the final version.

I believe the constraints are not defined in the right way. After generating code I got: ```python class TableRequirement(BaseModel): type: Literal[ 'assert-create', 'assert-table-uuid', 'assert-ref-snapshot-id', 'assert-last-assigned-field-id', 'assert-current-schema-id', 'assert-last-assigned-partition-id', 'assert-default-spec-id', 'assert-default-sort-order-id', ] ref: Optional[str] = None uuid: Optional[str] = None snapshot_id: Optional[int] = Field(None, alias='snapshot-id') last_assigned_field_id: Optional[int] = Field(None, alias='last-assigned-field-id') current_schema_id: Optional[int] = Field(None, alias='current-schema-id') last_assigned_partition_id: Optional[int] = Field( None, alias='last-assigned-partition-id' ) default_spec_id: Optional[int] = Field(None, alias='default-spec-id') default_sort_order_id: Optional[int] = Field(None, alias='default-sort-order-id') ``` Which encapulates all the requirements. After the refactor in this PR, we'll end up with: ```python class AssertCreate(BaseModel): type: Literal['assert-create'] class AssertTableUUID(BaseModel): type: Literal['assert-table-uuid'] uuid: str class AssertRefSnapshotId(BaseModel): type: Literal['assert-ref-snapshot-id'] ref: str snapshot_id: int = Field(..., alias='snapshot-id') class AssertLastAssignedFieldId(BaseModel): type: Literal['assert-last-assigned-field-id'] last_assigned_partition_id: int = Field(..., alias='last-assigned-partition-id') class AssertCurrentSchemaId(BaseModel): type: Literal['assert-current-schema-id'] current_schema_id: int = Field(..., alias='current-schema-id') class AssertLastAssignedPartitionId(BaseModel): type: Literal['assert-last-assigned-partition-id'] last_assigned_partition_id: int = Field(..., alias='last-assigned-partition-id') class AssertDefaultSpecId(BaseModel): type: Literal['assert-default-spec-id'] default_spec_id: int = Field(..., alias='default-spec-id') class AssertDefaultSortOrderId(BaseModel): type: Literal['assert-default-sort-order-id'] default_sort_order_id: int = Field(..., alias='default-sort-order-id') class TableRequirement(BaseModel): __root__: Union[ AssertCreate, AssertTableUUID, AssertRefSnapshotId, AssertLastAssignedFieldId, AssertCurrentSchemaId, AssertLastAssignedPartitionId, AssertDefaultSpecId, AssertDefaultSortOrderId, ] = Field(..., discriminator='type') ``` Which makes sense to me.

Co-authored-by: Eduard Tudenhoefner <etudenhoefner@gmail.com>

…ator-iceberg into fd-fix-open-api-requirements

Fokko · 2023-06-04T12:49:10Z

Alright, now I have it right:

class TableRequirement(BaseModel):
    type: str


class AssertCreate(TableRequirement):
    """
    The table must not already exist; used for create transactions
    """

    type: Literal['assert-create']


class AssertTableUUID(TableRequirement):
    """
    The table UUID must match the requirement's `uuid`
    """

    type: Literal['assert-table-uuid']
    uuid: str


class AssertRefSnapshotId(TableRequirement):
    """
    The table branch or tag identified by the requirement's `ref` must reference the requirement's `snapshot-id`; if `snapshot-id` is `null` or missing, the ref must not already exist
    """

    type: Literal['assert-ref-snapshot-id']
    ref: str
    snapshot_id: int = Field(..., alias='snapshot-id')


class AssertLastAssignedFieldId(TableRequirement):
    """
    The table's last assigned column id must match the requirement's `last-assigned-field-id`
    """

    type: Literal['assert-last-assigned-field-id']
    last_assigned_field_id: int = Field(..., alias='last-assigned-field-id')


class AssertCurrentSchemaId(TableRequirement):
    """
    The table's current schema id must match the requirement's `current-schema-id`
    """

    type: Literal['assert-current-schema-id']
    current_schema_id: int = Field(..., alias='current-schema-id')


class AssertLastAssignedPartitionId(TableRequirement):
    """
    The table's last assigned partition id must match the requirement's `last-assigned-partition-id`
    """

    type: Literal['assert-last-assigned-partition-id']
    last_assigned_partition_id: int = Field(..., alias='last-assigned-partition-id')


class AssertDefaultSpecId(TableRequirement):
    """
    The table's default spec id must match the requirement's `default-spec-id`
    """

    type: Literal['assert-default-spec-id']
    default_spec_id: int = Field(..., alias='default-spec-id')


class AssertDefaultSortOrderId(TableRequirement):
    """
    The table's default sort order id must match the requirement's `default-sort-order-id`
    """

    type: Literal['assert-default-sort-order-id']
    default_sort_order_id: int = Field(..., alias='default-sort-order-id')

The inheritance tree now checks out.

…pi-requirements

It is not super clear from the current structure: https://github.com/apache/iceberg/blob/main/open-api/rest-catalog-open-api.py#L304-L329 This is because I had to deconstruct the object. It would be great to get apache/iceberg#7710 in

rdblue · 2023-10-28T19:11:40Z

        - $ref: '#/components/schemas/SetPropertiesUpdate'
        - $ref: '#/components/schemas/RemovePropertiesUpdate'

-    TableRequirement:


Nit: Should TableRequirement be defined first?

Yes, I don't recall this isn't the case.

rdblue

This looks good to me. I like using the discriminator. We should probably refactor other areas to have that structure.

rdblue · 2023-10-28T19:12:52Z

Sorry for the late review. This looks great. Thanks @Fokko!

I didn't merge this in case it conflicts with the other linked PR. Feel free to merge in the order that makes the most sense.

Fokko · 2023-10-30T15:58:43Z

Thanks for the review @rdblue and @nastra for the review. I'll might revisitor other parts as well 👍

romusz · 2023-11-14T00:41:57Z

Hi,

Have you tested this refactoring with code generated for Go?
What we get now is:

type TableRequirement struct {
	Type string `json:"type"`
}

and (for example)

type AssertTableUUID struct {
	TableRequirement
	Type string `json:"type"`
	Uuid string `json:"uuid"`
}

There are couple of problems with that:

double storage of Type
provided []TableRequirement{} slice and the fact that TableRequirement is a struct not an interface, it cannot be "type switched" into it's "subtypes" (e.g. AssertTableUUID).
All previously-available, useful methods on TableRequirement have been nuked (e.g. GetUuidOk that allowed to extract TableUUID from assert-table-uuid requirement).

In summary, Go generated code does not produce equivalent to what it's shown for Python.
Please let me know if there is other way to traverse the requirements - so far it looks like a regression.

romusz · 2023-11-14T00:51:32Z

@Fokko not sure if you'll see the above comment since the pr has been merged, thus an explicit call-out.

nastra reviewed Jun 1, 2023

View reviewed changes

Comment thread open-api/rest-catalog-open-api.yaml Outdated

Comment thread open-api/rest-catalog-open-api.yaml Outdated

nastra reviewed Jun 1, 2023

View reviewed changes

Comment thread open-api/rest-catalog-open-api.yaml Outdated

Apply suggestions from code review

f4cbf76

Co-authored-by: Eduard Tudenhoefner <etudenhoefner@gmail.com>

github-actions Bot added the OPENAPI label Jun 4, 2023

Fokko mentioned this pull request Jun 4, 2023

Python: Alter table plumbing and REST support #6323

Merged

Fokko added 2 commits June 4, 2023 14:45

WIP

93724ec

Merge branch 'fd-fix-open-api-requirements' of github.com:Fokko/incub…

b30a0c9

…ator-iceberg into fd-fix-open-api-requirements

Fixed the inheritance

adda862

Fokko mentioned this pull request Jun 5, 2023

Open-API: Add Python code as an example #7751

Merged

Fokko added 3 commits June 6, 2023 11:44

Merge branch 'master' of github.com:apache/iceberg into fd-fix-open-a…

e8ed164

…pi-requirements

Merge branch 'master' of github.com:apache/iceberg into fd-fix-open-a…

4d54a30

…pi-requirements

Generate the code

5553dcd

Fokko mentioned this pull request Oct 25, 2023

Fix the nullability of snapshot-id on AssertRefSnapshotId apache/iceberg-python#103

Merged

rdblue reviewed Oct 28, 2023

View reviewed changes

rdblue approved these changes Oct 28, 2023

View reviewed changes

Fix order

9f6323a

Fokko merged commit a661663 into apache:main Oct 30, 2023

Fokko deleted the fd-fix-open-api-requirements branch October 30, 2023 15:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Open-API: Refactor TableRequirements#7710

Open-API: Refactor TableRequirements#7710
Fokko merged 9 commits intoapache:mainfrom
Fokko:fd-fix-open-api-requirements

Fokko commented May 26, 2023 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Fokko commented Jun 4, 2023 •

edited

Loading

Uh oh!

rdblue Oct 28, 2023

Uh oh!

Fokko Oct 30, 2023

Uh oh!

rdblue left a comment

Uh oh!

rdblue commented Oct 28, 2023

Uh oh!

Fokko commented Oct 30, 2023

Uh oh!

romusz commented Nov 14, 2023

Uh oh!

romusz commented Nov 14, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

Fokko commented May 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Fokko commented Jun 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rdblue Oct 28, 2023

Choose a reason for hiding this comment

Uh oh!

Fokko Oct 30, 2023

Choose a reason for hiding this comment

Uh oh!

rdblue left a comment

Choose a reason for hiding this comment

Uh oh!

rdblue commented Oct 28, 2023

Uh oh!

Fokko commented Oct 30, 2023

Uh oh!

romusz commented Nov 14, 2023

Uh oh!

romusz commented Nov 14, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Fokko commented May 26, 2023 •

edited

Loading

Fokko commented Jun 4, 2023 •

edited

Loading