You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "rutb327 (via GitHub)" <gi...@apache.org> on 2025/08/11 22:22:10 UTC
[PR] partition field names validation against schema field conflicts [iceberg-python]
rutb327 opened a new pull request, #2305:
URL: https://github.com/apache/iceberg-python/pull/2305
Closes [#2272](https://github.com/apache/iceberg-python/issues/2272)
Collaborator: @geruh
# Rationale for this change
Implements the validation logic described in [#2272](https://github.com/apache/iceberg-python/issues/2272) to match Java and Rust behavior for partition field name conflicts with schema fields.
This mirrors the method in Java checkAndAddPartitionName():
https://github.com/apache/iceberg/blob/4dbc7f578eee7ceb9def35ebfa1a4cc236fb598f/api/src/main/java/org/apache/iceberg/PartitionSpec.java#L392-L416
**Identity transforms** (`sourceColumnID != null`)- Allow schema field name conflicts only when sourced form the same field
**Non-identity** (`sourceColumnID == null`)- Disallow any schema field name conflicts.
In this PR `isinstance(transform, (IdentityTransform, VoidTransform))` is used to achieve the same logic as Java’s `sourceColumnID` check.
# Are these changes tested?
Yes, all existing tests pass and added a test covering validation scenarios.
# Are there any user-facing changes?
Yes. Non-identity transforms can no longer use schema field names as partition field names.
--
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
Re: [PR] partition field names validation against schema field conflicts [iceberg-python]
Posted by "rutb327 (via GitHub)" <gi...@apache.org>.
rutb327 commented on code in PR #2305:
URL: https://github.com/apache/iceberg-python/pull/2305#discussion_r2283080373
##########
pyiceberg/table/update/spec.py:
##########
@@ -244,6 +240,13 @@ def _add_new_field(
partition_fields.append(new_field)
for added_field in self._adds:
+ _check_and_add_partition_name(
+ self._transaction.table_metadata.schema(),
+ added_field.name,
+ added_field.source_id,
+ added_field.transform,
+ partition_names,
+ )
Review Comment:
yes, that's correct
--
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
Re: [PR] partition field names validation against schema field conflicts [iceberg-python]
Posted by "rutb327 (via GitHub)" <gi...@apache.org>.
rutb327 commented on code in PR #2305:
URL: https://github.com/apache/iceberg-python/pull/2305#discussion_r2283283747
##########
pyiceberg/table/update/schema.py:
##########
@@ -658,6 +658,14 @@ def _apply(self) -> Schema:
# Check the field-ids
new_schema = Schema(*struct.fields)
+ if self._transaction is not None:
+ from pyiceberg.partitioning import validate_partition_name
+
+ for spec in self._transaction.table_metadata.partition_specs:
+ for partition_field in spec.fields:
+ validate_partition_name(
+ partition_field.name, partition_field.transform, partition_field.source_id, new_schema
+ )
Review Comment:
Some tests show that transaction can be None in some cases, (after removing the check, tests from test_schema.py are failing). They use: `UpdateSchema(transaction=None, schema=Schema())`
[https://github.com/rutb327/iceberg-python/blob/24b12ddd8fdab4a62650786a2c3cdd56a53f8719/tests/test_schema.py#L933](url)
--
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
Re: [PR] partition field names validation against schema field conflicts [iceberg-python]
Posted by "rutb327 (via GitHub)" <gi...@apache.org>.
rutb327 commented on PR #2305:
URL: https://github.com/apache/iceberg-python/pull/2305#issuecomment-3180452864
In Java all partition-schema validation goes through https://github.com/apache/iceberg/blob/4dbc7f578eee7ceb9def35ebfa1a4cc236fb598f/api/src/main/java/org/apache/iceberg/PartitionSpec.java#L392-L416 during table creation with partition specs, partition spec updates and also during schema evolution.
In Python the validation in https://github.com/apache/iceberg-python/blob/d1c6005ad05166ab0fb08d3c15ccdfd7568e8013/pyiceberg/table/update/spec.py only covered partition spec updates
So, I've added the validation to:
- https://github.com/apache/iceberg-python/blob/d1c6005ad05166ab0fb08d3c15ccdfd7568e8013/pyiceberg/partitioning.py
- https://github.com/apache/iceberg-python/blob/d1c6005ad05166ab0fb08d3c15ccdfd7568e8013/pyiceberg/table/update/schema.py
**Are these the correct locations for the validation logic, or should they be placed elsewhere?**
--
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
Re: [PR] partition field names validation against schema field conflicts [iceberg-python]
Posted by "rutb327 (via GitHub)" <gi...@apache.org>.
rutb327 commented on code in PR #2305:
URL: https://github.com/apache/iceberg-python/pull/2305#discussion_r2289003854
##########
pyiceberg/partitioning.py:
##########
@@ -249,6 +249,31 @@ def partition_to_path(self, data: Record, schema: Schema) -> str:
UNPARTITIONED_PARTITION_SPEC = PartitionSpec(spec_id=0)
+def validate_partition_name(
+ field_name: str,
+ partition_transform: Transform[Any, Any],
+ source_id: int,
+ schema: Schema,
+ partition_names: Set[str],
+) -> None:
+ """Validate that a partition field name doesn't conflict with schema field names."""
+ try:
+ schema_field = schema.find_field(field_name)
+ except ValueError:
+ return # No conflict if field doesn't exist in schema
+
+ if isinstance(partition_transform, (IdentityTransform, VoidTransform)):
+ # For identity and void transforms, allow conflict only if sourced from the same schema field
+ if schema_field.field_id != source_id:
+ raise ValueError(f"Cannot create identity partition sourced from different field in schema: {field_name}")
+ else:
+ raise ValueError(f"Cannot create partition from name that exists in schema: {field_name}")
+ if not field_name:
+ raise ValueError("Undefined name")
Review Comment:
There can be empty strings right?
--
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
Re: [PR] partition field names validation against schema field conflicts [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #2305:
URL: https://github.com/apache/iceberg-python/pull/2305#discussion_r2287705324
##########
pyiceberg/partitioning.py:
##########
@@ -249,6 +249,31 @@ def partition_to_path(self, data: Record, schema: Schema) -> str:
UNPARTITIONED_PARTITION_SPEC = PartitionSpec(spec_id=0)
+def validate_partition_name(
+ field_name: str,
+ partition_transform: Transform[Any, Any],
+ source_id: int,
+ schema: Schema,
+ partition_names: Set[str],
+) -> None:
+ """Validate that a partition field name doesn't conflict with schema field names."""
+ try:
+ schema_field = schema.find_field(field_name)
+ except ValueError:
+ return # No conflict if field doesn't exist in schema
+
+ if isinstance(partition_transform, (IdentityTransform, VoidTransform)):
+ # For identity and void transforms, allow conflict only if sourced from the same schema field
+ if schema_field.field_id != source_id:
+ raise ValueError(f"Cannot create identity partition sourced from different field in schema: {field_name}")
+ else:
+ raise ValueError(f"Cannot create partition from name that exists in schema: {field_name}")
+ if not field_name:
+ raise ValueError("Undefined name")
Review Comment:
I don't think this will ever hit, since `field_name` is not `Optional`
--
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
Re: [PR] partition field names validation against schema field conflicts [iceberg-python]
Posted by "rutb327 (via GitHub)" <gi...@apache.org>.
rutb327 commented on code in PR #2305:
URL: https://github.com/apache/iceberg-python/pull/2305#discussion_r2289003854
##########
pyiceberg/partitioning.py:
##########
@@ -249,6 +249,31 @@ def partition_to_path(self, data: Record, schema: Schema) -> str:
UNPARTITIONED_PARTITION_SPEC = PartitionSpec(spec_id=0)
+def validate_partition_name(
+ field_name: str,
+ partition_transform: Transform[Any, Any],
+ source_id: int,
+ schema: Schema,
+ partition_names: Set[str],
+) -> None:
+ """Validate that a partition field name doesn't conflict with schema field names."""
+ try:
+ schema_field = schema.find_field(field_name)
+ except ValueError:
+ return # No conflict if field doesn't exist in schema
+
+ if isinstance(partition_transform, (IdentityTransform, VoidTransform)):
+ # For identity and void transforms, allow conflict only if sourced from the same schema field
+ if schema_field.field_id != source_id:
+ raise ValueError(f"Cannot create identity partition sourced from different field in schema: {field_name}")
+ else:
+ raise ValueError(f"Cannot create partition from name that exists in schema: {field_name}")
+ if not field_name:
+ raise ValueError("Undefined name")
Review Comment:
There needs to be a check for empty strings right?
--
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
Re: [PR] partition field names validation against schema field conflicts [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on PR #2305:
URL: https://github.com/apache/iceberg-python/pull/2305#issuecomment-3193961972
I opened https://github.com/apache/iceberg/issues/13833 and https://github.com/apache/iceberg-rust/issues/1609 for checking for name conflict during schema update
--
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
Re: [PR] partition field names validation against schema field conflicts [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #2305:
URL: https://github.com/apache/iceberg-python/pull/2305#discussion_r2287704596
##########
pyiceberg/partitioning.py:
##########
@@ -249,6 +249,31 @@ def partition_to_path(self, data: Record, schema: Schema) -> str:
UNPARTITIONED_PARTITION_SPEC = PartitionSpec(spec_id=0)
+def validate_partition_name(
+ field_name: str,
+ partition_transform: Transform[Any, Any],
+ source_id: int,
+ schema: Schema,
+ partition_names: Set[str],
+) -> None:
+ """Validate that a partition field name doesn't conflict with schema field names."""
+ try:
+ schema_field = schema.find_field(field_name)
+ except ValueError:
+ return # No conflict if field doesn't exist in schema
+
+ if isinstance(partition_transform, (IdentityTransform, VoidTransform)):
+ # For identity and void transforms, allow conflict only if sourced from the same schema field
+ if schema_field.field_id != source_id:
+ raise ValueError(f"Cannot create identity partition sourced from different field in schema: {field_name}")
+ else:
+ raise ValueError(f"Cannot create partition from name that exists in schema: {field_name}")
Review Comment:
```suggestion
raise ValueError(f"Cannot create partition with a name that exists in schema: {field_name}")
```
--
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
Re: [PR] partition field names validation against schema field conflicts [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on PR #2305:
URL: https://github.com/apache/iceberg-python/pull/2305#issuecomment-3208019497
Let's move this forward, thanks @rutb327 for working on this, and thanks @kevinjqliu and @dingo4dev for the review 🙌
--
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
Re: [PR] partition field names validation against schema field conflicts [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on code in PR #2305:
URL: https://github.com/apache/iceberg-python/pull/2305#discussion_r2284124849
##########
pyiceberg/table/update/schema.py:
##########
@@ -658,6 +658,14 @@ def _apply(self) -> Schema:
# Check the field-ids
new_schema = Schema(*struct.fields)
+ if self._transaction is not None:
+ from pyiceberg.partitioning import validate_partition_name
+
+ for spec in self._transaction.table_metadata.partition_specs:
+ for partition_field in spec.fields:
+ validate_partition_name(
+ partition_field.name, partition_field.transform, partition_field.source_id, new_schema
+ )
Review Comment:
looks like everywhere else in the codebase we include transaction in UpdateSchema.
Maybe we can update the tests like this
```
def test_add_top_level_primitives(primitive_fields: List[NestedField], table_v2: Table) -> None:
for primitive_field in primitive_fields:
new_schema = Schema(primitive_field)
applied = UpdateSchema(transaction=Transaction(table_v2), schema=Schema()).union_by_name(new_schema)._apply() # type: ignore
assert applied == new_schema
```
##########
tests/integration/test_partition_evolution.py:
##########
@@ -63,13 +63,22 @@ def _table_v2(catalog: Catalog) -> Table:
return _create_table_with_schema(catalog, schema_with_timestamp, "2")
-def _create_table_with_schema(catalog: Catalog, schema: Schema, format_version: str) -> Table:
+def _create_table_with_schema(
+ catalog: Catalog, schema: Schema, format_version: str, partition_spec: PartitionSpec = UNPARTITIONED_PARTITION_SPEC
+) -> Table:
tbl_name = "default.test_schema_evolution"
try:
catalog.drop_table(tbl_name)
except NoSuchTableError:
pass
- return catalog.create_table(identifier=tbl_name, schema=schema, properties={"format-version": format_version})
+
+ if partition_spec:
+ return catalog.create_table(
+ identifier=tbl_name, schema=schema, partition_spec=partition_spec, properties={"format-version": format_version}
+ )
+ return catalog.create_table(
+ identifier=tbl_name, schema=schema, partition_spec=partition_spec, properties={"format-version": format_version}
+ )
Review Comment:
```suggestion
return catalog.create_table(
identifier=tbl_name, schema=schema, partition_spec=partition_spec, properties={"format-version": format_version}
)
```
--
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
Re: [PR] partition field names validation against schema field conflicts [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on code in PR #2305:
URL: https://github.com/apache/iceberg-python/pull/2305#discussion_r2280604003
##########
tests/integration/test_partition_evolution.py:
##########
@@ -63,12 +64,19 @@ def _table_v2(catalog: Catalog) -> Table:
return _create_table_with_schema(catalog, schema_with_timestamp, "2")
-def _create_table_with_schema(catalog: Catalog, schema: Schema, format_version: str) -> Table:
+def _create_table_with_schema(
+ catalog: Catalog, schema: Schema, format_version: str, partition_spec: Optional[PartitionSpec] = None
+) -> Table:
tbl_name = "default.test_schema_evolution"
try:
catalog.drop_table(tbl_name)
except NoSuchTableError:
pass
+
+ if partition_spec:
+ return catalog.create_table(
+ identifier=tbl_name, schema=schema, partition_spec=partition_spec, properties={"format-version": format_version}
+ )
return catalog.create_table(identifier=tbl_name, schema=schema, properties={"format-version": format_version})
Review Comment:
and then we can just do this
```suggestion
return catalog.create_table(
identifier=tbl_name, schema=schema, partition_spec=partition_spec, properties={"format-version": format_version}
)
```
##########
pyiceberg/table/update/spec.py:
##########
@@ -174,16 +174,12 @@ def _commit(self) -> UpdatesAndRequirements:
return updates, requirements
def _apply(self) -> PartitionSpec:
- def _check_and_add_partition_name(schema: Schema, name: str, source_id: int, partition_names: Set[str]) -> None:
- try:
- field = schema.find_field(name)
- except ValueError:
- field = None
-
- if source_id is not None and field is not None and field.field_id != source_id:
- raise ValueError(f"Cannot create identity partition from a different field in the schema {name}")
- elif field is not None and source_id != field.field_id:
- raise ValueError(f"Cannot create partition from name that exists in schema {name}")
+ def _check_and_add_partition_name(
+ schema: Schema, name: str, source_id: int, transform: Transform[Any, Any], partition_names: Set[str]
+ ) -> None:
+ from pyiceberg.partitioning import validate_partition_name
+
+ validate_partition_name(name, transform, source_id, schema)
if not name:
Review Comment:
wdyt about moving L183-L186 into the `validate_partition_name` to mirror the java impl
https://github.com/apache/iceberg/blob/4dbc7f578eee7ceb9def35ebfa1a4cc236fb598f/api/src/main/java/org/apache/iceberg/PartitionSpec.java#L412-L414
##########
tests/integration/test_partition_evolution.py:
##########
@@ -63,12 +64,19 @@ def _table_v2(catalog: Catalog) -> Table:
return _create_table_with_schema(catalog, schema_with_timestamp, "2")
-def _create_table_with_schema(catalog: Catalog, schema: Schema, format_version: str) -> Table:
+def _create_table_with_schema(
+ catalog: Catalog, schema: Schema, format_version: str, partition_spec: Optional[PartitionSpec] = None
+) -> Table:
Review Comment:
following other create table helpers in tests, for example
https://github.com/apache/iceberg-python/blob/80135451d030569259d83674ef147e0d6f62fd51/tests/integration/test_register_table.py#L40-L59
```suggestion
def _create_table_with_schema(
catalog: Catalog, schema: Schema, format_version: str, partition_spec: PartitionSpec = UNPARTITIONED_PARTITION_SPEC
) -> Table:
```
##########
pyiceberg/partitioning.py:
##########
@@ -249,6 +249,26 @@ def partition_to_path(self, data: Record, schema: Schema) -> str:
UNPARTITIONED_PARTITION_SPEC = PartitionSpec(spec_id=0)
+def validate_partition_name(
+ field_name: str,
+ partition_transform: Transform[Any, Any],
+ source_id: int,
+ schema: Schema,
+) -> None:
+ """Validate that a partition field name doesn't conflict with schema field names."""
+ try:
+ schema_field = schema.find_field(field_name)
+ except ValueError:
+ return # No conflict if field doesn't exist in schema
+
+ if isinstance(partition_transform, (IdentityTransform, VoidTransform)):
+ # For identity transforms, allow conflict only if sourced from the same schema field
+ if schema_field.field_id != source_id:
+ raise ValueError(f"Cannot create identity partition from a different source field in the schema: {field_name}")
+ else:
Review Comment:
match java error message
```suggestion
raise ValueError(f"Cannot create identity partition sourced from different field in schema: {field_name}")
else:
```
##########
pyiceberg/table/update/spec.py:
##########
@@ -244,6 +240,13 @@ def _add_new_field(
partition_fields.append(new_field)
for added_field in self._adds:
+ _check_and_add_partition_name(
+ self._transaction.table_metadata.schema(),
+ added_field.name,
+ added_field.source_id,
+ added_field.transform,
+ partition_names,
+ )
Review Comment:
good catch. just to confirm this covers the newly added partition fields?
##########
pyiceberg/partitioning.py:
##########
@@ -249,6 +249,26 @@ def partition_to_path(self, data: Record, schema: Schema) -> str:
UNPARTITIONED_PARTITION_SPEC = PartitionSpec(spec_id=0)
+def validate_partition_name(
+ field_name: str,
+ partition_transform: Transform[Any, Any],
+ source_id: int,
+ schema: Schema,
+) -> None:
+ """Validate that a partition field name doesn't conflict with schema field names."""
+ try:
+ schema_field = schema.find_field(field_name)
+ except ValueError:
+ return # No conflict if field doesn't exist in schema
+
+ if isinstance(partition_transform, (IdentityTransform, VoidTransform)):
+ # For identity transforms, allow conflict only if sourced from the same schema field
Review Comment:
```suggestion
# For identity and void transforms, allow conflict only if sourced from the same schema field
```
##########
pyiceberg/table/update/schema.py:
##########
@@ -658,6 +658,14 @@ def _apply(self) -> Schema:
# Check the field-ids
new_schema = Schema(*struct.fields)
+ if self._transaction is not None:
+ from pyiceberg.partitioning import validate_partition_name
+
+ for spec in self._transaction.table_metadata.partition_specs:
+ for partition_field in spec.fields:
+ validate_partition_name(
+ partition_field.name, partition_field.transform, partition_field.source_id, new_schema
+ )
Review Comment:
i think there should always be a `self._transaction`
```suggestion
from pyiceberg.partitioning import validate_partition_name
for spec in self._transaction.table_metadata.partition_specs:
for partition_field in spec.fields:
validate_partition_name(
partition_field.name, partition_field.transform, partition_field.source_id, new_schema
)
```
--
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
Re: [PR] partition field names validation against schema field conflicts [iceberg-python]
Posted by "rutb327 (via GitHub)" <gi...@apache.org>.
rutb327 commented on code in PR #2305:
URL: https://github.com/apache/iceberg-python/pull/2305#discussion_r2283110191
##########
pyiceberg/table/update/schema.py:
##########
@@ -658,6 +658,14 @@ def _apply(self) -> Schema:
# Check the field-ids
new_schema = Schema(*struct.fields)
+ if self._transaction is not None:
+ from pyiceberg.partitioning import validate_partition_name
+
+ for spec in self._transaction.table_metadata.partition_specs:
+ for partition_field in spec.fields:
+ validate_partition_name(
+ partition_field.name, partition_field.transform, partition_field.source_id, new_schema
+ )
Review Comment:
okay, I'll do the suggested changes
##########
pyiceberg/table/update/spec.py:
##########
@@ -174,16 +174,12 @@ def _commit(self) -> UpdatesAndRequirements:
return updates, requirements
def _apply(self) -> PartitionSpec:
- def _check_and_add_partition_name(schema: Schema, name: str, source_id: int, partition_names: Set[str]) -> None:
- try:
- field = schema.find_field(name)
- except ValueError:
- field = None
-
- if source_id is not None and field is not None and field.field_id != source_id:
- raise ValueError(f"Cannot create identity partition from a different field in the schema {name}")
- elif field is not None and source_id != field.field_id:
- raise ValueError(f"Cannot create partition from name that exists in schema {name}")
+ def _check_and_add_partition_name(
+ schema: Schema, name: str, source_id: int, transform: Transform[Any, Any], partition_names: Set[str]
+ ) -> None:
+ from pyiceberg.partitioning import validate_partition_name
+
+ validate_partition_name(name, transform, source_id, schema)
if not name:
Review Comment:
We can do that
--
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
Re: [PR] partition field names validation against schema field conflicts [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko merged PR #2305:
URL: https://github.com/apache/iceberg-python/pull/2305
--
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
Re: [PR] partition field names validation against schema field conflicts [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #2305:
URL: https://github.com/apache/iceberg-python/pull/2305#discussion_r2289210827
##########
pyiceberg/partitioning.py:
##########
@@ -249,6 +249,31 @@ def partition_to_path(self, data: Record, schema: Schema) -> str:
UNPARTITIONED_PARTITION_SPEC = PartitionSpec(spec_id=0)
+def validate_partition_name(
+ field_name: str,
+ partition_transform: Transform[Any, Any],
+ source_id: int,
+ schema: Schema,
+ partition_names: Set[str],
+) -> None:
+ """Validate that a partition field name doesn't conflict with schema field names."""
+ try:
+ schema_field = schema.find_field(field_name)
+ except ValueError:
+ return # No conflict if field doesn't exist in schema
+
+ if isinstance(partition_transform, (IdentityTransform, VoidTransform)):
+ # For identity and void transforms, allow conflict only if sourced from the same schema field
+ if schema_field.field_id != source_id:
+ raise ValueError(f"Cannot create identity partition sourced from different field in schema: {field_name}")
+ else:
+ raise ValueError(f"Cannot create partition from name that exists in schema: {field_name}")
+ if not field_name:
+ raise ValueError("Undefined name")
Review Comment:
Ah, yes! Thanks!
--
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