You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "hililiwei (via GitHub)" <gi...@apache.org> on 2023/08/25 09:57:33 UTC
[GitHub] [iceberg] hililiwei opened a new pull request, #8393: Python: Check if identifier fields are valid
hililiwei opened a new pull request, #8393:
URL: https://github.com/apache/iceberg/pull/8393
Fix #8386
Note: This pr only did validity checks when creating the Schema, not when it evolves, need to integrate with https://github.com/apache/iceberg/pull/8374.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] hililiwei commented on a diff in pull request #8393: Python: Check if identifier fields are valid
Posted by "hililiwei (via GitHub)" <gi...@apache.org>.
hililiwei commented on code in PR #8393:
URL: https://github.com/apache/iceberg/pull/8393#discussion_r1306347722
##########
python/pyiceberg/schema.py:
##########
@@ -273,6 +277,49 @@ def field_ids(self) -> Set[int]:
"""Returns the IDs of the current schema."""
return set(self._name_to_id.values())
+ @staticmethod
+ def validate_identifier_field(field_id: int, id_to_field: Dict[int, NestedField], id_to_parent: Dict[int, int]) -> None:
+ """Validates that the field with the given ID is a valid identifier field.
+
+ Args:
+ field_id: The ID of the field to validate.
+ id_to_field: A map from field IDs to field objects.
+ id_to_parent: A map from field IDs to their parent field IDs.
+
+ Raises:
+ ValueError: If the field is not valid.
+ """
+ field = id_to_field.get(field_id)
+ if field is None:
+ raise ValueError(f"Cannot add fieldId {field_id} as an identifier field: field does not exist")
+
+ if not field.field_type.is_primitive:
+ raise ValueError(f"Cannot add field {field.name} as an identifier field: not a primitive type field")
+
+ if not field.required:
+ raise ValueError(f"Cannot add field {field.name} as an identifier field: not a required field")
+
+ if isinstance(field.field_type, DoubleType) or isinstance(field.field_type, FloatType):
+ raise ValueError(f"Cannot add field {field.name} as an identifier field: must not be float or double field")
+
+ # Check whether the nested field is in a chain of required struct fields
+ # Exploring from root for better error message for list and map types
+ parent_id = id_to_parent.get(field.field_id)
+ fields: List[int] = []
+ while parent_id is not None:
Review Comment:
The reason for this is to explore from root.
As the comment says: `# Exploring from root for better error message for list and map types`
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] Fokko commented on a diff in pull request #8393: Python: Check if identifier fields are valid
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #8393:
URL: https://github.com/apache/iceberg/pull/8393#discussion_r1305661994
##########
python/pyiceberg/schema.py:
##########
@@ -273,6 +277,49 @@ def field_ids(self) -> Set[int]:
"""Returns the IDs of the current schema."""
return set(self._name_to_id.values())
+ @staticmethod
+ def validate_identifier_field(field_id: int, id_to_field: Dict[int, NestedField], id_to_parent: Dict[int, int]) -> None:
+ """Validates that the field with the given ID is a valid identifier field.
+
+ Args:
+ field_id: The ID of the field to validate.
+ id_to_field: A map from field IDs to field objects.
+ id_to_parent: A map from field IDs to their parent field IDs.
+
+ Raises:
+ ValueError: If the field is not valid.
+ """
+ field = id_to_field.get(field_id)
+ if field is None:
+ raise ValueError(f"Cannot add fieldId {field_id} as an identifier field: field does not exist")
+
+ if not field.field_type.is_primitive:
+ raise ValueError(f"Cannot add field {field.name} as an identifier field: not a primitive type field")
+
+ if not field.required:
+ raise ValueError(f"Cannot add field {field.name} as an identifier field: not a required field")
+
+ if isinstance(field.field_type, DoubleType) or isinstance(field.field_type, FloatType):
Review Comment:
These can be combined:
```suggestion
if isinstance(field.field_type, (DoubleType, FloatType)):
```
##########
python/pyiceberg/schema.py:
##########
@@ -273,6 +277,49 @@ def field_ids(self) -> Set[int]:
"""Returns the IDs of the current schema."""
return set(self._name_to_id.values())
+ @staticmethod
+ def validate_identifier_field(field_id: int, id_to_field: Dict[int, NestedField], id_to_parent: Dict[int, int]) -> None:
+ """Validates that the field with the given ID is a valid identifier field.
+
+ Args:
+ field_id: The ID of the field to validate.
+ id_to_field: A map from field IDs to field objects.
+ id_to_parent: A map from field IDs to their parent field IDs.
+
+ Raises:
+ ValueError: If the field is not valid.
+ """
+ field = id_to_field.get(field_id)
+ if field is None:
+ raise ValueError(f"Cannot add fieldId {field_id} as an identifier field: field does not exist")
+
+ if not field.field_type.is_primitive:
+ raise ValueError(f"Cannot add field {field.name} as an identifier field: not a primitive type field")
+
+ if not field.required:
+ raise ValueError(f"Cannot add field {field.name} as an identifier field: not a required field")
+
+ if isinstance(field.field_type, DoubleType) or isinstance(field.field_type, FloatType):
+ raise ValueError(f"Cannot add field {field.name} as an identifier field: must not be float or double field")
+
+ # Check whether the nested field is in a chain of required struct fields
+ # Exploring from root for better error message for list and map types
+ parent_id = id_to_parent.get(field.field_id)
+ fields: List[int] = []
+ while parent_id is not None:
Review Comment:
More of a style question. Do we first want to store it into `fields` we could also already do the check.
##########
python/pyiceberg/schema.py:
##########
@@ -882,6 +929,57 @@ def index_by_id(schema_or_type: Union[Schema, IcebergType]) -> Dict[int, NestedF
return visit(schema_or_type, _IndexById())
+class _IndexParents(SchemaVisitor[Dict[int, int]]):
+ def __init__(self) -> None:
+ self.id_to_parent: Dict[int, int] = {}
+ self.id_stack: List[int] = []
+
+ def before_field(self, field: NestedField) -> None:
+ self.id_stack.append(field.field_id)
+
+ def after_field(self, field: NestedField) -> None:
+ self.id_stack.pop()
+
+ def schema(self, schema: Schema, struct_result: Dict[int, int]) -> Dict[int, int]:
+ return self.id_to_parent
+
+ def struct(self, struct: StructType, field_results: List[Dict[int, int]]) -> Dict[int, int]:
+ for field in struct.fields:
+ parent_id = self.id_stack[-1] if self.id_stack else None
+ if parent_id is not None:
+ # fields in the root struct are not added
+ self.id_to_parent[field.field_id] = parent_id
+
+ return self.id_to_parent
+
+ def field(self, field: NestedField, field_result: Dict[int, int]) -> Dict[int, int]:
+ return self.id_to_parent
+
+ def list(self, list_type: ListType, element_result: Dict[int, int]) -> Dict[int, int]:
+ self.id_to_parent[list_type.element_id] = self.id_stack[-1]
+ return self.id_to_parent
+
+ def map(self, map_type: MapType, key_result: Dict[int, int], value_result: Dict[int, int]) -> Dict[int, int]:
+ self.id_to_parent[map_type.key_id] = self.id_stack[-1]
+ self.id_to_parent[map_type.value_id] = self.id_stack[-1]
+ return self.id_to_parent
+
+ def primitive(self, primitive: PrimitiveType) -> Dict[int, int]:
+ return self.id_to_parent
+
+
+def index_parents(schema_or_type: Union[Schema, IcebergType]) -> Dict[int, int]:
Review Comment:
Should we make this one private to the module? I don't think we want to use this outside of `schema.py`
```suggestion
def _index_parents(schema_or_type: Union[Schema, IcebergType]) -> Dict[int, int]:
```
##########
python/pyiceberg/schema.py:
##########
@@ -88,6 +88,10 @@ def __init__(self, *fields: NestedField, **data: Any):
data["fields"] = fields
super().__init__(**data)
self._name_to_id = index_by_name(self)
+ if self.identifier_field_ids is not None:
Review Comment:
Since `Schema` derives from `IcebergBaseModel`, we can use Pydantic validators: https://github.com/apache/iceberg/blob/master/python/pyiceberg/table/metadata.py#L239-L241
With an after validator, we already check if the field is a `List[int]`, and when it is none, the `default_factory=list` is applied.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] Fokko commented on pull request #8393: Python: Check if identifier fields are valid
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on PR #8393:
URL: https://github.com/apache/iceberg/pull/8393#issuecomment-1695428726
This is great, thanks @hililiwei for picking this up! 🚀
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] hililiwei commented on a diff in pull request #8393: Python: Check if identifier fields are valid
Posted by "hililiwei (via GitHub)" <gi...@apache.org>.
hililiwei commented on code in PR #8393:
URL: https://github.com/apache/iceberg/pull/8393#discussion_r1306347495
##########
python/pyiceberg/schema.py:
##########
@@ -882,6 +929,57 @@ def index_by_id(schema_or_type: Union[Schema, IcebergType]) -> Dict[int, NestedF
return visit(schema_or_type, _IndexById())
+class _IndexParents(SchemaVisitor[Dict[int, int]]):
+ def __init__(self) -> None:
+ self.id_to_parent: Dict[int, int] = {}
+ self.id_stack: List[int] = []
+
+ def before_field(self, field: NestedField) -> None:
+ self.id_stack.append(field.field_id)
+
+ def after_field(self, field: NestedField) -> None:
+ self.id_stack.pop()
+
+ def schema(self, schema: Schema, struct_result: Dict[int, int]) -> Dict[int, int]:
+ return self.id_to_parent
+
+ def struct(self, struct: StructType, field_results: List[Dict[int, int]]) -> Dict[int, int]:
+ for field in struct.fields:
+ parent_id = self.id_stack[-1] if self.id_stack else None
+ if parent_id is not None:
+ # fields in the root struct are not added
+ self.id_to_parent[field.field_id] = parent_id
+
+ return self.id_to_parent
+
+ def field(self, field: NestedField, field_result: Dict[int, int]) -> Dict[int, int]:
+ return self.id_to_parent
+
+ def list(self, list_type: ListType, element_result: Dict[int, int]) -> Dict[int, int]:
+ self.id_to_parent[list_type.element_id] = self.id_stack[-1]
+ return self.id_to_parent
+
+ def map(self, map_type: MapType, key_result: Dict[int, int], value_result: Dict[int, int]) -> Dict[int, int]:
+ self.id_to_parent[map_type.key_id] = self.id_stack[-1]
+ self.id_to_parent[map_type.value_id] = self.id_stack[-1]
+ return self.id_to_parent
+
+ def primitive(self, primitive: PrimitiveType) -> Dict[int, int]:
+ return self.id_to_parent
+
+
+def index_parents(schema_or_type: Union[Schema, IcebergType]) -> Dict[int, int]:
Review Comment:
We may need to use it in `table/__init__.py` when we do schema evolution.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] Fokko merged pull request #8393: Python: Check if identifier fields are valid
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko merged PR #8393:
URL: https://github.com/apache/iceberg/pull/8393
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] hililiwei commented on a diff in pull request #8393: Python: Check if identifier fields are valid
Posted by "hililiwei (via GitHub)" <gi...@apache.org>.
hililiwei commented on code in PR #8393:
URL: https://github.com/apache/iceberg/pull/8393#discussion_r1306347722
##########
python/pyiceberg/schema.py:
##########
@@ -273,6 +277,49 @@ def field_ids(self) -> Set[int]:
"""Returns the IDs of the current schema."""
return set(self._name_to_id.values())
+ @staticmethod
+ def validate_identifier_field(field_id: int, id_to_field: Dict[int, NestedField], id_to_parent: Dict[int, int]) -> None:
+ """Validates that the field with the given ID is a valid identifier field.
+
+ Args:
+ field_id: The ID of the field to validate.
+ id_to_field: A map from field IDs to field objects.
+ id_to_parent: A map from field IDs to their parent field IDs.
+
+ Raises:
+ ValueError: If the field is not valid.
+ """
+ field = id_to_field.get(field_id)
+ if field is None:
+ raise ValueError(f"Cannot add fieldId {field_id} as an identifier field: field does not exist")
+
+ if not field.field_type.is_primitive:
+ raise ValueError(f"Cannot add field {field.name} as an identifier field: not a primitive type field")
+
+ if not field.required:
+ raise ValueError(f"Cannot add field {field.name} as an identifier field: not a required field")
+
+ if isinstance(field.field_type, DoubleType) or isinstance(field.field_type, FloatType):
+ raise ValueError(f"Cannot add field {field.name} as an identifier field: must not be float or double field")
+
+ # Check whether the nested field is in a chain of required struct fields
+ # Exploring from root for better error message for list and map types
+ parent_id = id_to_parent.get(field.field_id)
+ fields: List[int] = []
+ while parent_id is not None:
Review Comment:
The reason for this is to explore from root.
As the 306L comment says: `# Exploring from root for better error message for list and map types`
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] hililiwei commented on pull request #8393: Python: Check if identifier fields are valid
Posted by "hililiwei (via GitHub)" <gi...@apache.org>.
hililiwei commented on PR #8393:
URL: https://github.com/apache/iceberg/pull/8393#issuecomment-1696757504
Thanks @Fokko !
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] Fokko commented on a diff in pull request #8393: Python: Check if identifier fields are valid
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #8393:
URL: https://github.com/apache/iceberg/pull/8393#discussion_r1307236910
##########
python/tests/test_schema.py:
##########
@@ -724,6 +726,52 @@ def should_promote(file_type: IcebergType, read_type: IcebergType) -> bool:
return False
+def test_identifier_fields_fails(table_schema_nested_with_struct_key_map: Schema) -> None:
+ with pytest.raises(ValueError) as exc_info:
+ Schema(*table_schema_nested_with_struct_key_map.fields, schema_id=1, identifier_field_ids=[999])
+ assert "Could not find field with id: 999" in str(exc_info.value)
+
+ with pytest.raises(ValueError) as exc_info:
+ Schema(*table_schema_nested_with_struct_key_map.fields, schema_id=1, identifier_field_ids=[11])
+ assert "Identifier field 11 invalid: not a primitive type field" in str(exc_info.value)
+
+ with pytest.raises(ValueError) as exc_info:
+ Schema(*table_schema_nested_with_struct_key_map.fields, schema_id=1, identifier_field_ids=[3])
+ assert "Identifier field 3 invalid: not a required field" in str(exc_info.value)
+
+ with pytest.raises(ValueError) as exc_info:
+ Schema(*table_schema_nested_with_struct_key_map.fields, schema_id=1, identifier_field_ids=[28])
+ assert "Identifier field 28 invalid: must not be float or double field" in str(exc_info.value)
+
+ with pytest.raises(ValueError) as exc_info:
+ Schema(*table_schema_nested_with_struct_key_map.fields, schema_id=1, identifier_field_ids=[29])
+ assert "Identifier field 29 invalid: must not be float or double field" in str(exc_info.value)
+
+ with pytest.raises(ValueError) as exc_info:
+ Schema(*table_schema_nested_with_struct_key_map.fields, schema_id=1, identifier_field_ids=[23])
+ assert (
+ "Cannot add field zip as an identifier field: must not be nested in %s"
+ % table_schema_nested_with_struct_key_map.find_field("location")
+ in str(exc_info.value)
+ )
+
+ with pytest.raises(ValueError) as exc_info:
+ Schema(*table_schema_nested_with_struct_key_map.fields, schema_id=1, identifier_field_ids=[26])
+ assert (
+ "Cannot add field x as an identifier field: must not be nested in %s"
Review Comment:
Nit: I prefer f-strings
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] Fokko commented on a diff in pull request #8393: Python: Check if identifier fields are valid
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #8393:
URL: https://github.com/apache/iceberg/pull/8393#discussion_r1306487300
##########
python/pyiceberg/schema.py:
##########
@@ -273,6 +282,49 @@ def field_ids(self) -> Set[int]:
"""Returns the IDs of the current schema."""
return set(self._name_to_id.values())
+ @staticmethod
+ def validate_identifier_field(field_id: int, id_to_field: Dict[int, NestedField], id_to_parent: Dict[int, int]) -> None:
+ """Validates that the field with the given ID is a valid identifier field.
+
+ Args:
+ field_id: The ID of the field to validate.
+ id_to_field: A map from field IDs to field objects.
+ id_to_parent: A map from field IDs to their parent field IDs.
+
+ Raises:
+ ValueError: If the field is not valid.
+ """
+ field = id_to_field.get(field_id)
+ if field is None:
+ raise ValueError(f"Cannot add fieldId {field_id} as an identifier field: field does not exist")
Review Comment:
```suggestion
raise ValueError(f"Identifier field {field_id} invalid: does not exist")
```
##########
python/pyiceberg/schema.py:
##########
@@ -273,6 +282,49 @@ def field_ids(self) -> Set[int]:
"""Returns the IDs of the current schema."""
return set(self._name_to_id.values())
+ @staticmethod
+ def validate_identifier_field(field_id: int, id_to_field: Dict[int, NestedField], id_to_parent: Dict[int, int]) -> None:
Review Comment:
Why not make this one non-static and private:
```suggestion
def _validate_identifier_field(self, field_id: int) -> None:
```
This way we can just call `self.find_field`
##########
python/pyiceberg/schema.py:
##########
@@ -273,6 +282,49 @@ def field_ids(self) -> Set[int]:
"""Returns the IDs of the current schema."""
return set(self._name_to_id.values())
+ @staticmethod
+ def validate_identifier_field(field_id: int, id_to_field: Dict[int, NestedField], id_to_parent: Dict[int, int]) -> None:
+ """Validates that the field with the given ID is a valid identifier field.
+
+ Args:
+ field_id: The ID of the field to validate.
+ id_to_field: A map from field IDs to field objects.
+ id_to_parent: A map from field IDs to their parent field IDs.
+
+ Raises:
+ ValueError: If the field is not valid.
+ """
+ field = id_to_field.get(field_id)
+ if field is None:
+ raise ValueError(f"Cannot add fieldId {field_id} as an identifier field: field does not exist")
+
+ if not field.field_type.is_primitive:
+ raise ValueError(f"Cannot add field {field.name} as an identifier field: not a primitive type field")
Review Comment:
```suggestion
raise ValueError(f"Identifier field {field_id} invalid: not a primitive type field")
```
##########
python/pyiceberg/schema.py:
##########
@@ -88,6 +88,10 @@ def __init__(self, *fields: NestedField, **data: Any):
data["fields"] = fields
super().__init__(**data)
self._name_to_id = index_by_name(self)
+ if self.identifier_field_ids is not None:
Review Comment:
That looks great! Having this in `schema.py` is the right place
##########
python/pyiceberg/schema.py:
##########
@@ -273,6 +282,49 @@ def field_ids(self) -> Set[int]:
"""Returns the IDs of the current schema."""
return set(self._name_to_id.values())
+ @staticmethod
+ def validate_identifier_field(field_id: int, id_to_field: Dict[int, NestedField], id_to_parent: Dict[int, int]) -> None:
+ """Validates that the field with the given ID is a valid identifier field.
+
+ Args:
+ field_id: The ID of the field to validate.
+ id_to_field: A map from field IDs to field objects.
+ id_to_parent: A map from field IDs to their parent field IDs.
+
+ Raises:
+ ValueError: If the field is not valid.
+ """
+ field = id_to_field.get(field_id)
+ if field is None:
+ raise ValueError(f"Cannot add fieldId {field_id} as an identifier field: field does not exist")
+
+ if not field.field_type.is_primitive:
+ raise ValueError(f"Cannot add field {field.name} as an identifier field: not a primitive type field")
+
+ if not field.required:
+ raise ValueError(f"Cannot add field {field.name} as an identifier field: not a required field")
+
+ if isinstance(field.field_type, (DoubleType, FloatType)):
+ raise ValueError(f"Cannot add field {field.name} as an identifier field: must not be float or double field")
Review Comment:
```suggestion
raise ValueError(f"Identifier field {field_id} invalid: must not be float or double field")
```
##########
python/pyiceberg/schema.py:
##########
@@ -273,6 +282,49 @@ def field_ids(self) -> Set[int]:
"""Returns the IDs of the current schema."""
return set(self._name_to_id.values())
+ @staticmethod
+ def validate_identifier_field(field_id: int, id_to_field: Dict[int, NestedField], id_to_parent: Dict[int, int]) -> None:
+ """Validates that the field with the given ID is a valid identifier field.
+
+ Args:
+ field_id: The ID of the field to validate.
+ id_to_field: A map from field IDs to field objects.
+ id_to_parent: A map from field IDs to their parent field IDs.
+
+ Raises:
+ ValueError: If the field is not valid.
+ """
+ field = id_to_field.get(field_id)
+ if field is None:
+ raise ValueError(f"Cannot add fieldId {field_id} as an identifier field: field does not exist")
+
+ if not field.field_type.is_primitive:
+ raise ValueError(f"Cannot add field {field.name} as an identifier field: not a primitive type field")
+
+ if not field.required:
+ raise ValueError(f"Cannot add field {field.name} as an identifier field: not a required field")
Review Comment:
```suggestion
raise ValueError(f"Identifier field {field_id} invalid: not a required field")
```
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] hililiwei commented on a diff in pull request #8393: Python: Check if identifier fields are valid
Posted by "hililiwei (via GitHub)" <gi...@apache.org>.
hililiwei commented on code in PR #8393:
URL: https://github.com/apache/iceberg/pull/8393#discussion_r1307141619
##########
python/pyiceberg/schema.py:
##########
@@ -273,6 +282,49 @@ def field_ids(self) -> Set[int]:
"""Returns the IDs of the current schema."""
return set(self._name_to_id.values())
+ @staticmethod
+ def validate_identifier_field(field_id: int, id_to_field: Dict[int, NestedField], id_to_parent: Dict[int, int]) -> None:
Review Comment:
Updated. Please take a look.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] hililiwei commented on a diff in pull request #8393: Python: Check if identifier fields are valid
Posted by "hililiwei (via GitHub)" <gi...@apache.org>.
hililiwei commented on code in PR #8393:
URL: https://github.com/apache/iceberg/pull/8393#discussion_r1307141167
##########
python/pyiceberg/schema.py:
##########
@@ -882,6 +929,57 @@ def index_by_id(schema_or_type: Union[Schema, IcebergType]) -> Dict[int, NestedF
return visit(schema_or_type, _IndexById())
+class _IndexParents(SchemaVisitor[Dict[int, int]]):
+ def __init__(self) -> None:
+ self.id_to_parent: Dict[int, int] = {}
+ self.id_stack: List[int] = []
+
+ def before_field(self, field: NestedField) -> None:
+ self.id_stack.append(field.field_id)
+
+ def after_field(self, field: NestedField) -> None:
+ self.id_stack.pop()
+
+ def schema(self, schema: Schema, struct_result: Dict[int, int]) -> Dict[int, int]:
+ return self.id_to_parent
+
+ def struct(self, struct: StructType, field_results: List[Dict[int, int]]) -> Dict[int, int]:
+ for field in struct.fields:
+ parent_id = self.id_stack[-1] if self.id_stack else None
+ if parent_id is not None:
+ # fields in the root struct are not added
+ self.id_to_parent[field.field_id] = parent_id
+
+ return self.id_to_parent
+
+ def field(self, field: NestedField, field_result: Dict[int, int]) -> Dict[int, int]:
+ return self.id_to_parent
+
+ def list(self, list_type: ListType, element_result: Dict[int, int]) -> Dict[int, int]:
+ self.id_to_parent[list_type.element_id] = self.id_stack[-1]
+ return self.id_to_parent
+
+ def map(self, map_type: MapType, key_result: Dict[int, int], value_result: Dict[int, int]) -> Dict[int, int]:
+ self.id_to_parent[map_type.key_id] = self.id_stack[-1]
+ self.id_to_parent[map_type.value_id] = self.id_stack[-1]
+ return self.id_to_parent
+
+ def primitive(self, primitive: PrimitiveType) -> Dict[int, int]:
+ return self.id_to_parent
+
+
+def index_parents(schema_or_type: Union[Schema, IcebergType]) -> Dict[int, int]:
Review Comment:
That makes sense.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] Fokko commented on a diff in pull request #8393: Python: Check if identifier fields are valid
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #8393:
URL: https://github.com/apache/iceberg/pull/8393#discussion_r1306486592
##########
python/pyiceberg/schema.py:
##########
@@ -882,6 +929,57 @@ def index_by_id(schema_or_type: Union[Schema, IcebergType]) -> Dict[int, NestedF
return visit(schema_or_type, _IndexById())
+class _IndexParents(SchemaVisitor[Dict[int, int]]):
+ def __init__(self) -> None:
+ self.id_to_parent: Dict[int, int] = {}
+ self.id_stack: List[int] = []
+
+ def before_field(self, field: NestedField) -> None:
+ self.id_stack.append(field.field_id)
+
+ def after_field(self, field: NestedField) -> None:
+ self.id_stack.pop()
+
+ def schema(self, schema: Schema, struct_result: Dict[int, int]) -> Dict[int, int]:
+ return self.id_to_parent
+
+ def struct(self, struct: StructType, field_results: List[Dict[int, int]]) -> Dict[int, int]:
+ for field in struct.fields:
+ parent_id = self.id_stack[-1] if self.id_stack else None
+ if parent_id is not None:
+ # fields in the root struct are not added
+ self.id_to_parent[field.field_id] = parent_id
+
+ return self.id_to_parent
+
+ def field(self, field: NestedField, field_result: Dict[int, int]) -> Dict[int, int]:
+ return self.id_to_parent
+
+ def list(self, list_type: ListType, element_result: Dict[int, int]) -> Dict[int, int]:
+ self.id_to_parent[list_type.element_id] = self.id_stack[-1]
+ return self.id_to_parent
+
+ def map(self, map_type: MapType, key_result: Dict[int, int], value_result: Dict[int, int]) -> Dict[int, int]:
+ self.id_to_parent[map_type.key_id] = self.id_stack[-1]
+ self.id_to_parent[map_type.value_id] = self.id_stack[-1]
+ return self.id_to_parent
+
+ def primitive(self, primitive: PrimitiveType) -> Dict[int, int]:
+ return self.id_to_parent
+
+
+def index_parents(schema_or_type: Union[Schema, IcebergType]) -> Dict[int, int]:
Review Comment:
Ah, I think it makes more sense to access it through schema, for example `schema.field_parent_id(1)`, similar to `find_column_name`. A nice thing here is that we can catch the dict so we don't have to compute it every time.
```python
@cached_property
def _lazy_id_to_name(self) -> Dict[int, str]:
```
This dict will then be created if there are identifier fields, otherwise, the first time we access it.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] hililiwei commented on a diff in pull request #8393: Python: Check if identifier fields are valid
Posted by "hililiwei (via GitHub)" <gi...@apache.org>.
hililiwei commented on code in PR #8393:
URL: https://github.com/apache/iceberg/pull/8393#discussion_r1307206090
##########
python/pyiceberg/schema.py:
##########
@@ -882,6 +929,57 @@ def index_by_id(schema_or_type: Union[Schema, IcebergType]) -> Dict[int, NestedF
return visit(schema_or_type, _IndexById())
+class _IndexParents(SchemaVisitor[Dict[int, int]]):
+ def __init__(self) -> None:
+ self.id_to_parent: Dict[int, int] = {}
+ self.id_stack: List[int] = []
+
+ def before_field(self, field: NestedField) -> None:
+ self.id_stack.append(field.field_id)
+
+ def after_field(self, field: NestedField) -> None:
+ self.id_stack.pop()
+
+ def schema(self, schema: Schema, struct_result: Dict[int, int]) -> Dict[int, int]:
+ return self.id_to_parent
+
+ def struct(self, struct: StructType, field_results: List[Dict[int, int]]) -> Dict[int, int]:
+ for field in struct.fields:
+ parent_id = self.id_stack[-1] if self.id_stack else None
+ if parent_id is not None:
+ # fields in the root struct are not added
+ self.id_to_parent[field.field_id] = parent_id
+
+ return self.id_to_parent
+
+ def field(self, field: NestedField, field_result: Dict[int, int]) -> Dict[int, int]:
+ return self.id_to_parent
+
+ def list(self, list_type: ListType, element_result: Dict[int, int]) -> Dict[int, int]:
+ self.id_to_parent[list_type.element_id] = self.id_stack[-1]
+ return self.id_to_parent
+
+ def map(self, map_type: MapType, key_result: Dict[int, int], value_result: Dict[int, int]) -> Dict[int, int]:
+ self.id_to_parent[map_type.key_id] = self.id_stack[-1]
+ self.id_to_parent[map_type.value_id] = self.id_stack[-1]
+ return self.id_to_parent
+
+ def primitive(self, primitive: PrimitiveType) -> Dict[int, int]:
+ return self.id_to_parent
+
+
+def index_parents(schema_or_type: Union[Schema, IcebergType]) -> Dict[int, int]:
Review Comment:
I created `_lazy_id_to_parent`. https://github.com/apache/iceberg/pull/8393/files#diff-514b42afa20282d1ad0e3e2518ceb390ca79cb675d66c43bd1d51eac2b96d05fR142-R147
I'm not sure if we should create a method like `find_field_parent (filed_int: int)`?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] hililiwei commented on a diff in pull request #8393: Python: Check if identifier fields are valid
Posted by "hililiwei (via GitHub)" <gi...@apache.org>.
hililiwei commented on code in PR #8393:
URL: https://github.com/apache/iceberg/pull/8393#discussion_r1306350653
##########
python/pyiceberg/schema.py:
##########
@@ -88,6 +88,10 @@ def __init__(self, *fields: NestedField, **data: Any):
data["fields"] = fields
super().__init__(**data)
self._name_to_id = index_by_name(self)
+ if self.identifier_field_ids is not None:
Review Comment:
I tried adding a validator in `schema.py`. Please take a look.
Or should I add a validator like `check_schema` in `metadata.py`?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org