Bug Fix: Rest Catalog update partition spec and sort order when fresh schema is created

[sungwy]

Right now, if fresh schema IDs are assigned on the Iceberg table schema that doesn't map exactly to the schema that was originally passed in, the partition and sort order may map to a different field ID.

When fresh schema ID is assigned, the partition spec and sort order must also be generated based on the fresh schema before CreateTableRequest is sent to the rest catalog.

Thank you @jqin61 for identifying the issue!

[kevinjqliu]

do you know if this is captured by the REST integration test? would be a good testcase if not

[sungwy]

do you know if this is captured by the REST integration test? would be a good testcase if not

Yes, I'm in the process of writing it up now :)

Thank you for the suggestion @kevinjqliu

[kevinjqliu]

btw, i have a toy REST catalog here. its easier than spinning up the integration docker image every time.

~/.pyiceberg.yaml

catalog:
  default:
    uri: https://iceberg-rest-image.fly.dev

Points to this
https://iceberg-rest-image.fly.dev/v1/namespaces/default/tables/taxi_dataset

the only thing in the catalog now is from the "getting started" guide

[kevinjqliu]

some comments on equality check.

Also, it looks like we are reassigning ids from 1. I'm not familiar with this process so bare with me. Is there a reason why we can't create the new table with the original schema, starting at id 4 & 5?

[kevinjqliu]

nit, should we also check tbl.schema().schema_id since Schema's __eq__ doesn't check for that

iceberg-python/pyiceberg/schema.py

Lines 104 to 118 in cec051f

	def __eq__(self, other: Any) -> bool:
	"""Return the equality of two instances of the Schema class."""
	if not other:
	return False
	if not isinstance(other, Schema):
	return False
	if len(self.columns) != len(other.columns):
	return False
	identifier_field_ids_is_equal = self.identifier_field_ids == other.identifier_field_ids
	schema_is_equal = all(lhs == rhs for lhs, rhs in zip(self.columns, other.columns))
	return identifier_field_ids_is_equal and schema_is_equal

[sungwy]

Sure. What do you think about just asserting that the schema_id is 0? Since it will always be 0 on creation?

[kevinjqliu]

PartitionSpec's __eq__ checks for the spec_id

iceberg-python/pyiceberg/partitioning.py

Lines 110 to 119 in cec051f

	def __eq__(self, other: Any) -> bool:
	"""
	Produce a boolean to return True if two objects are considered equal.
	Note:
	Equality of PartitionSpec is determined by spec_id and partition fields only.
	"""
	if not isinstance(other, PartitionSpec):
	return False
	return self.spec_id == other.spec_id and self.fields == other.fields

[kevinjqliu]

SortOrder doesn't seem to have a __eq__ function defined.

iceberg-python/pyiceberg/table/sorting.py

Lines 127 to 164 in cec051f

	class SortOrder(IcebergBaseModel):
	"""Describes how the data is sorted within the table.
	Users can sort their data within partitions by columns to gain performance.
	The order of the sort fields within the list defines the order in which the sort is applied to the data.
	Args:
	fields (List[SortField]): The fields how the table is sorted.
	Keyword Args:
	order_id (int): An unique id of the sort-order of a table.
	"""
	order_id: int = Field(alias="order-id", default=INITIAL_SORT_ORDER_ID)
	fields: List[SortField] = Field(default_factory=list)
	def __init__(self, *fields: SortField, **data: Any):
	if fields:
	data["fields"] = fields
	super().__init__(**data)
	@property
	def is_unsorted(self) -> bool:
	return len(self.fields) == 0
	def __str__(self) -> str:
	"""Return the string representation of the SortOrder class."""
	result_str = "["
	if self.fields:
	result_str += "\n " + "\n ".join([str(field) for field in self.fields]) + "\n"
	result_str += "]"
	return result_str
	def __repr__(self) -> str:
	"""Return the string representation of the SortOrder class."""
	fields = f"{', '.join(repr(column) for column in self.fields)}, " if self.fields else ""
	return f"SortOrder({fields}order_id={self.order_id})"

What is the behavior here?

[sungwy]

I believe it would invoke the eq of IcebergBaseModel -> BaseModel, which checks the equality of all of the fields of the BaseModel

[sungwy]

some comments on equality check.

Also, it looks like we are reassigning ids from 1. I'm not familiar with this process so bare with me. Is there a reason why we can't create the new table with the original schema, starting at id 4 & 5?

I think we inherit this logic from the java code. When a new table is created, simply a new table is created - so we just assign IDs in pre-order traversal order

[HonahX]

LGTM! Thanks @syun64 for working on this and @kevinjqliu for reviewing.

Also, it looks like we are reassigning ids from 1. I'm not familiar with this process so bare with me. Is there a reason why we can't create the new table with the original schema, starting at id 4 & 5?

Adding to @syun64's explanation, doing this in RestCatalog Client is safer option in case the server implementation does not do this by default: #327 (review)

[Fokko]

Good catch @syun64 thanks for fixing this 👍