Fokko commented on code in PR #10434:
URL: https://github.com/apache/iceberg/pull/10434#discussion_r1625486376
##########
open-api/rest-catalog-open-api.py:
##########
@@ -361,23 +361,29 @@ class RemovePartitionStatisticsUpdate(BaseUpdate):
class TableRequirement(BaseModel):
- type: str
+ 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',
+ ]
class AssertCreate(TableRequirement):
"""
The table must not already exist; used for create transactions
"""
- type: Literal['assert-create']
Review Comment:
It looks like we're using it in an odd way here. The example
[here](https://docs.pydantic.dev/latest/concepts/unions/#discriminated-unions-with-str-discriminators)
also does not use inheritance but returns an union:
```python
class Cat(BaseModel):
pet_type: Literal['cat']
meows: int
class Dog(BaseModel):
pet_type: Literal['dog']
barks: float
class Lizard(BaseModel):
pet_type: Literal['reptile', 'lizard']
scales: bool
class Model(BaseModel):
pet: Union[Cat, Dog, Lizard] = Field(..., discriminator='pet_type')
n: int
```
It is just that we try to add this field in there. I think the following is
more correct:
```yaml
diff --git a/open-api/rest-catalog-open-api.yaml
b/open-api/rest-catalog-open-api.yaml
index f8f68d06df..b85207e57b 100644
--- a/open-api/rest-catalog-open-api.yaml
+++ b/open-api/rest-catalog-open-api.yaml
@@ -2589,18 +2589,6 @@ components:
type: object
required:
- type
- properties:
- type:
- type: "string"
- enum:
- - "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"
discriminator:
propertyName: type
mapping:
@@ -2612,6 +2600,15 @@ components:
assert-last-assigned-partition-id:
'#/components/schemas/AssertLastAssignedPartitionId'
assert-default-spec-id: '#/components/schemas/AssertDefaultSpecId'
assert-default-sort-order-id:
'#/components/schemas/AssertDefaultSortOrderId'
+ oneOf:
+ - $ref: '#/components/schemas/AssertCreate'
+ - $ref: '#/components/schemas/AssertTableUUID'
+ - $ref: '#/components/schemas/AssertRefSnapshotId'
+ - $ref: '#/components/schemas/AssertLastAssignedFieldId'
+ - $ref: '#/components/schemas/AssertCurrentSchemaId'
+ - $ref: '#/components/schemas/AssertLastAssignedPartitionId'
+ - $ref: '#/components/schemas/AssertDefaultSpecId'
+ - $ref: '#/components/schemas/AssertDefaultSortOrderId'
AssertCreate:
allOf:
@@ -2636,6 +2633,7 @@ components:
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
required:
+ - type
- ref
- snapshot-id
properties:
@@ -2651,8 +2649,12 @@ components:
description:
The table's last assigned column id must match the requirement's
`last-assigned-field-id`
required:
+ - type
- last-assigned-field-id
properties:
+ type:
+ type: string
+ enum: [ "assert-last-assigned-field-id" ]
last-assigned-field-id:
type: integer
@@ -2662,8 +2664,12 @@ components:
description:
The table's current schema id must match the requirement's
`current-schema-id`
required:
+ - type
- current-schema-id
properties:
+ type:
+ type: string
+ enum: [ "assert-current-schema-id" ]
current-schema-id:
type: integer
@@ -2673,8 +2679,12 @@ components:
description:
The table's last assigned partition id must match the requirement's
`last-assigned-partition-id`
required:
+ - type
- last-assigned-partition-id
properties:
+ type:
+ type: string
+ enum: [ "assert-last-assigned-partition-id" ]
last-assigned-partition-id:
type: integer
@@ -2684,8 +2694,12 @@ components:
description:
The table's default spec id must match the requirement's
`default-spec-id`
required:
+ - type
- default-spec-id
properties:
+ type:
+ type: string
+ enum: [ "assert-default-spec-id" ]
default-spec-id:
type: integer
@@ -2695,8 +2709,12 @@ components:
description:
The table's default sort order id must match the requirement's
`default-sort-order-id`
required:
+ - type
- default-sort-order-id
properties:
+ type:
+ type: string
+ enum: [ "assert-default-sort-order-id" ]
default-sort-order-id:
type: integer
```
This yields (just one example for readability):
```python
+class TableRequirement(BaseModel):
+ __root__: Union[
+ AssertCreate,
+ AssertTableUUID,
+ AssertRefSnapshotId,
+ AssertLastAssignedFieldId,
+ AssertCurrentSchemaId,
+ AssertLastAssignedPartitionId,
+ AssertDefaultSpecId,
+ AssertDefaultSortOrderId,
+ ] = Field(..., discriminator='type')
+
+
+class AssertCreate(BaseModel):
+ """
+ The table must not already exist; used for create transactions
+ """
+
+ type: Literal['assert-create']
```
The `__root__` means that the class itself is one of these listed in the
`Union`. This would be more in line with the official open-API documentation
(example at the bottom of the page):
https://swagger.io/docs/specification/data-models/inheritance-and-polymorphism/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
