Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 3.5k
Python: Fine-tune the API#5672
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
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
83ae7aadd28be8a0806cdac27b893a5e1658fa8d8809c6c49e76e9a544c72ac0614692File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -104,6 +104,9 @@ def __str__(self): | ||
| return f"{self.transform}({self.source_id}) {self.direction} {self.null_order}" | ||
| INITIAL_SORT_ORDER_ID = 1 | ||
| class SortOrder(IcebergBaseModel): | ||
| """Describes how the data is sorted within the table | ||
| @@ -112,20 +115,18 @@ class SortOrder(IcebergBaseModel): | ||
| The order of the sort fields within the list defines the order in which the sort is applied to the data. | ||
| Args: | ||
| order_id (int): The id of the sort-order. To keep track of historical sorting | ||
| order_id (int): An unique id of the sort-orderof a table. | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we need "of a table" -- that assumes the context that uses the sort order. | ||
| fields (List[SortField]): The fields how the table is sorted | ||
| """ | ||
| def __init__(self, order_id: Optional[int] = None, *fields: SortField, **data: Any): | ||
| if order_id is not None: | ||
| data["order-id"] = order_id | ||
| order_id: int = Field(alias="order-id", default=INITIAL_SORT_ORDER_ID) | ||
rdblue marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| fields: List[SortField] = Field(default_factory=list) | ||
| def __init__(self, *fields: SortField, **data: Any): | ||
| if fields: | ||
| data["fields"] = fields | ||
| super().__init__(**data) | ||
| order_id: int = Field(alias="order-id") | ||
| fields: List[SortField] = Field(default_factory=list) | ||
| @property | ||
| def is_unsorted(self) -> bool: | ||
| return len(self.fields) == 0 | ||
| @@ -137,10 +138,13 @@ def __str__(self) -> str: | ||
| result_str += "]" | ||
| return result_str | ||
| def __repr__(self): | ||
| fields = f"{', '.join(repr(column) for column in self.fields)}, " if self.fields else "" | ||
| return f"SortOrder({fields}order_id={self.order_id})" | ||
| UNSORTED_SORT_ORDER_ID = 0 | ||
| UNSORTED_SORT_ORDER = SortOrder(order_id=UNSORTED_SORT_ORDER_ID) | ||
| INITIAL_SORT_ORDER_ID = 1 | ||
| def assign_fresh_sort_order_ids(sort_order: SortOrder, old_schema: Schema, fresh_schema: Schema) -> SortOrder: | ||
| @@ -164,7 +168,4 @@ def assign_fresh_sort_order_ids(sort_order: SortOrder, old_schema: Schema, fresh | ||
| ) | ||
| ) | ||
| return SortOrder( | ||
| INITIAL_SORT_ORDER_ID, | ||
| *fresh_fields, | ||
| ) | ||
| return SortOrder(*fresh_fields, order_id=INITIAL_SORT_ORDER_ID) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -344,8 +344,8 @@ def test_load_table_200(rest_mock: Mocker): | ||
| }, | ||
| status_code=200, | ||
| ) | ||
| table = RestCatalog("rest", uri=TEST_URI, token=TEST_TOKEN).load_table(("fokko", "table")) | ||
| assert table == Table( | ||
| actual = RestCatalog("rest", uri=TEST_URI, token=TEST_TOKEN).load_table(("fokko", "table")) | ||
| expected = Table( | ||
| identifier=("rest", "fokko", "table"), | ||
| metadata_location="s3://warehouse/database/table/metadata/00001-5f2f8166-244c-4eae-ac36-384ecdec81fc.gz.metadata.json", | ||
| metadata=TableMetadataV1( | ||
| @@ -362,7 +362,6 @@ def test_load_table_200(rest_mock: Mocker): | ||
| ) | ||
| ], | ||
| current_schema_id=0, | ||
| partition_specs=[PartitionSpec(spec_id=0, fields=())], | ||
| default_spec_id=0, | ||
| last_partition_id=999, | ||
| properties={"owner": "bryan", "write.metadata.compression-codec": "gzip"}, | ||
| @@ -422,6 +421,7 @@ def test_load_table_200(rest_mock: Mocker): | ||
| ), | ||
| config={"client.factory": "io.tabular.iceberg.catalog.TabularAwsClientFactory", "region": "us-west-2"}, | ||
| ) | ||
| assert actual == expected | ||
| def test_load_table_404(rest_mock: Mocker): | ||
| @@ -496,7 +496,6 @@ def test_create_table_200(rest_mock: Mocker, table_schema_simple: Schema): | ||
| ], | ||
| "partition-spec": [], | ||
| "default-spec-id": 0, | ||
| "partition-specs": [{"spec-id": 0, "fields": []}], | ||
| "last-partition-id": 999, | ||
| "default-sort-order-id": 0, | ||
| "sort-orders": [{"order-id": 0, "fields": []}], | ||
| @@ -524,9 +523,9 @@ def test_create_table_200(rest_mock: Mocker, table_schema_simple: Schema): | ||
| schema=table_schema_simple, | ||
| location=None, | ||
| partition_spec=PartitionSpec( | ||
| spec_id=1, fields=(PartitionField(source_id=1, field_id=1000, transform=TruncateTransform(width=3), name="id"),) | ||
| PartitionField(source_id=1, field_id=1000, transform=TruncateTransform(width=3), name="id"), spec_id=1 | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like the mock causes the result to not match the request. We should start testing against the REST catalog servlet as soon as we can. | ||
| ), | ||
| sort_order=SortOrder(1, SortField(source_id=2, transform=IdentityTransform())), | ||
| sort_order=SortOrder(SortField(source_id=2, transform=IdentityTransform())), | ||
| properties={"owner": "fokko"}, | ||
| ) | ||
| assert table == Table( | ||
| @@ -547,7 +546,6 @@ def test_create_table_200(rest_mock: Mocker, table_schema_simple: Schema): | ||
| ) | ||
| ], | ||
| current_schema_id=0, | ||
| partition_specs=[PartitionSpec(spec_id=0, fields=())], | ||
| default_spec_id=0, | ||
| last_partition_id=999, | ||
| properties={ | ||
| @@ -595,10 +593,9 @@ def test_create_table_409(rest_mock, table_schema_simple: Schema): | ||
| schema=table_schema_simple, | ||
| location=None, | ||
| partition_spec=PartitionSpec( | ||
| spec_id=1, | ||
| fields=(PartitionField(source_id=1, field_id=1000, transform=TruncateTransform(width=3), name="id"),), | ||
| PartitionField(source_id=1, field_id=1000, transform=TruncateTransform(width=3), name="id") | ||
| ), | ||
| sort_order=SortOrder(1, SortField(source_id=2, transform=IdentityTransform())), | ||
| sort_order=SortOrder(SortField(source_id=2, transform=IdentityTransform())), | ||
| properties={"owner": "fokko"}, | ||
| ) | ||
| assert "Table already exists" in str(e.value) | ||
Uh oh!
There was an error while loading. Please reload this page.
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.
I think I'd prefer to handle ID assignment manually rather than defaulting. Defaulting seems to bring in complexity because if we forget to pass along an ID somewhere, it would cause problems.
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.
I feel that we don't should really expose this to the user. For example, when we create a new table, we re-assign the IDs anyway (using the assign fresh IDs logic).
If we follow the Java API, and we have something similar to
updateSpec: https://github.com/apache/iceberg/blob/master/api/src/main/java/org/apache/iceberg/Table.java#L165-L171 Then we can just take the next ID. What do you think of this?Uh oh!
There was an error while loading. Please reload this page.
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.
That sounds reasonable to me. I think we just need to make sure that reassignment is correct!
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.
This will definitely involve a lot of testing 👍🏻