You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "Fokko (via GitHub)" <gi...@apache.org> on 2025/08/06 18:26:46 UTC
[PR] Convert `_get_column_projection_values` to use Field-IDs [iceberg-python]
Fokko opened a new pull request, #2293:
URL: https://github.com/apache/iceberg-python/pull/2293
# Rationale for this change
This is a refactor of the `_get_column_projection_values` to rely on field-IDs rather than names. Field IDs will never change, while partitions and column names can be updated in a tables' lifetime.
# Are these changes tested?
# Are there any user-facing changes?
<!-- In the case of user-facing changes, please add the changelog label. -->
--
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] Convert `_get_column_projection_values` to use Field-IDs [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #2293:
URL: https://github.com/apache/iceberg-python/pull/2293#discussion_r2257960677
##########
pyiceberg/expressions/visitors.py:
##########
@@ -915,8 +916,10 @@ def visit_bound_predicate(self, predicate: BoundPredicate[L]) -> BooleanExpressi
# In the order described by the "Column Projection" section of the Iceberg spec:
# https://iceberg.apache.org/spec/#column-projection
# Evaluate column projection first if it exists
- if projected_field_value := self.projected_field_values.get(field.name):
Review Comment:
Removing wallrus `:=` here since the projected value can also be `None`
--
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] Convert `_get_column_projection_values` to use Field-IDs [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on PR #2293:
URL: https://github.com/apache/iceberg-python/pull/2293#issuecomment-3162652246
> Should we also address this comment in the PR? https://github.com/apache/iceberg-python/pull/2029#issuecomment-3152625389
Yes we can do that here as well, however, I think it is an extreme edge-case. This is only related to when the partition-spec is set, but the actual fields in the struct are not set. This potentially can cause data-incorrectness in V1 tables (because there the partition struct doesn't have field-IDs). I'm pretty sure that any Iceberg client won't produce such data, but it could be in the case of a poorly written add-files script that ignores partitioning.
--
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] Convert `_get_column_projection_values` to use Field-IDs [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #2293:
URL: https://github.com/apache/iceberg-python/pull/2293#discussion_r2257958637
##########
tests/io/test_pyarrow.py:
##########
@@ -1189,7 +1189,7 @@ def test_identity_transform_column_projection(tmp_path: str, catalog: InMemoryCa
with transaction.update_snapshot().overwrite() as update:
update.append_data_file(unpartitioned_file)
- schema = pa.schema([("other_field", pa.string()), ("partition_id", pa.int64())])
Review Comment:
The `IdentityTransform` returns the same type as the one in the table:
```python
schema = Schema(
NestedField(1, "other_field", StringType(), required=False), NestedField(2, "partition_id", IntegerType(), required=False)
)
```
--
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] Convert `_get_column_projection_values` to use Field-IDs [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on PR #2293:
URL: https://github.com/apache/iceberg-python/pull/2293#issuecomment-3162658995
Thanks @kevinjqliu 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] Convert `_get_column_projection_values` to use Field-IDs [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #2293:
URL: https://github.com/apache/iceberg-python/pull/2293#discussion_r2258454236
##########
pyiceberg/io/pyarrow.py:
##########
@@ -1401,42 +1400,24 @@ def _field_id(self, field: pa.Field) -> int:
def _get_column_projection_values(
- file: DataFile, projected_schema: Schema, partition_spec: Optional[PartitionSpec], file_project_field_ids: Set[int]
-) -> Tuple[bool, Dict[str, Any]]:
+ file: DataFile, projected_schema: Schema, partition_spec: PartitionSpec, file_project_field_ids: Set[int]
+) -> Dict[int, Any]:
"""Apply Column Projection rules to File Schema."""
project_schema_diff = projected_schema.field_ids.difference(file_project_field_ids)
- should_project_columns = len(project_schema_diff) > 0
- projected_missing_fields: Dict[str, Any] = {}
+ if len(project_schema_diff) == 0:
+ return EMPTY_DICT
- if not should_project_columns:
- return False, {}
-
- partition_schema: StructType
- accessors: Dict[int, Accessor]
-
- if partition_spec is not None:
- partition_schema = partition_spec.partition_type(projected_schema)
- accessors = build_position_accessors(partition_schema)
- else:
- return False, {}
+ partition_schema = partition_spec.partition_type(projected_schema)
+ accessors = build_position_accessors(partition_schema)
+ projected_missing_fields = {}
for field_id in project_schema_diff:
for partition_field in partition_spec.fields_by_source_id(field_id):
if isinstance(partition_field.transform, IdentityTransform):
- accessor = accessors.get(partition_field.field_id)
-
- if accessor is None:
- continue
-
- # The partition field may not exist in the partition record of the data file.
- # This can happen when new partition fields are introduced after the file was written.
- try:
- if partition_value := accessor.get(file.partition):
- projected_missing_fields[partition_field.name] = partition_value
- except IndexError:
- continue
+ if partition_value := accessors[partition_field.field_id].get(file.partition):
+ projected_missing_fields[field_id] = partition_value
Review Comment:
This was actually a bug. It always used the current spec, while we should use the spec that it was written with.
--
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] Convert `_get_column_projection_values` to use Field-IDs [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on PR #2293:
URL: https://github.com/apache/iceberg-python/pull/2293#issuecomment-3161521160
should we also address this comment in the PR?
https://github.com/apache/iceberg-python/pull/2029#issuecomment-3152625389
--
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] Convert `_get_column_projection_values` to use Field-IDs [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #2293:
URL: https://github.com/apache/iceberg-python/pull/2293#discussion_r2257966076
##########
pyiceberg/io/pyarrow.py:
##########
@@ -1401,42 +1400,24 @@ def _field_id(self, field: pa.Field) -> int:
def _get_column_projection_values(
- file: DataFile, projected_schema: Schema, partition_spec: Optional[PartitionSpec], file_project_field_ids: Set[int]
-) -> Tuple[bool, Dict[str, Any]]:
+ file: DataFile, projected_schema: Schema, partition_spec: PartitionSpec, file_project_field_ids: Set[int]
+) -> Dict[int, Any]:
"""Apply Column Projection rules to File Schema."""
project_schema_diff = projected_schema.field_ids.difference(file_project_field_ids)
- should_project_columns = len(project_schema_diff) > 0
- projected_missing_fields: Dict[str, Any] = {}
+ if len(project_schema_diff) == 0:
+ return EMPTY_DICT
- if not should_project_columns:
- return False, {}
-
- partition_schema: StructType
- accessors: Dict[int, Accessor]
-
- if partition_spec is not None:
- partition_schema = partition_spec.partition_type(projected_schema)
- accessors = build_position_accessors(partition_schema)
- else:
- return False, {}
+ partition_schema = partition_spec.partition_type(projected_schema)
+ accessors = build_position_accessors(partition_schema)
+ projected_missing_fields = {}
for field_id in project_schema_diff:
for partition_field in partition_spec.fields_by_source_id(field_id):
if isinstance(partition_field.transform, IdentityTransform):
- accessor = accessors.get(partition_field.field_id)
-
- if accessor is None:
- continue
-
- # The partition field may not exist in the partition record of the data file.
- # This can happen when new partition fields are introduced after the file was written.
- try:
- if partition_value := accessor.get(file.partition):
- projected_missing_fields[partition_field.name] = partition_value
- except IndexError:
- continue
+ if partition_value := accessors[partition_field.field_id].get(file.partition):
+ projected_missing_fields[field_id] = partition_value
Review Comment:
I think it makes sense to fail here, rather than suppress the Error. In the case of an `IndexError` I think your table is corrupt.
--
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] Convert `_get_column_projection_values` to use Field-IDs [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #2293:
URL: https://github.com/apache/iceberg-python/pull/2293#discussion_r2257956398
##########
tests/expressions/test_visitors.py:
##########
@@ -1730,6 +1730,33 @@ def test_translate_column_names_missing_column_match_null() -> None:
assert translated_expr == AlwaysTrue()
+def test_translate_column_names_missing_column_match_explicit_null() -> None:
+ """Test translate_column_names when missing column matches null."""
+ # Original schema
+ original_schema = Schema(
+ NestedField(field_id=1, name="existing_col", field_type=StringType(), required=False),
+ NestedField(field_id=2, name="missing_col", field_type=IntegerType(), required=False),
+ schema_id=1,
+ )
+
+ # Create bound expression for the missing column
+ unbound_expr = IsNull("missing_col")
+ bound_expr = visit(unbound_expr, visitor=BindVisitor(schema=original_schema, case_sensitive=True))
+
+ # File schema only has the existing column (field_id=1), missing field_id=2
+ file_schema = Schema(
+ NestedField(field_id=1, name="existing_col", field_type=StringType(), required=False),
+ schema_id=1,
+ )
+
+ # Translate column names
+ translated_expr = translate_column_names(bound_expr, file_schema, case_sensitive=True, projected_field_values={2: None})
Review Comment:
A partition can be null as well 👍
--
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] Convert `_get_column_projection_values` to use Field-IDs [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko merged PR #2293:
URL: https://github.com/apache/iceberg-python/pull/2293
--
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] Convert `_get_column_projection_values` to use Field-IDs [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on code in PR #2293:
URL: https://github.com/apache/iceberg-python/pull/2293#discussion_r2258601170
##########
pyiceberg/io/pyarrow.py:
##########
@@ -1401,42 +1400,24 @@ def _field_id(self, field: pa.Field) -> int:
def _get_column_projection_values(
- file: DataFile, projected_schema: Schema, partition_spec: Optional[PartitionSpec], file_project_field_ids: Set[int]
-) -> Tuple[bool, Dict[str, Any]]:
+ file: DataFile, projected_schema: Schema, partition_spec: PartitionSpec, file_project_field_ids: Set[int]
+) -> Dict[int, Any]:
"""Apply Column Projection rules to File Schema."""
project_schema_diff = projected_schema.field_ids.difference(file_project_field_ids)
- should_project_columns = len(project_schema_diff) > 0
- projected_missing_fields: Dict[str, Any] = {}
+ if len(project_schema_diff) == 0:
+ return EMPTY_DICT
- if not should_project_columns:
- return False, {}
-
- partition_schema: StructType
- accessors: Dict[int, Accessor]
-
- if partition_spec is not None:
- partition_schema = partition_spec.partition_type(projected_schema)
- accessors = build_position_accessors(partition_schema)
- else:
- return False, {}
+ partition_schema = partition_spec.partition_type(projected_schema)
+ accessors = build_position_accessors(partition_schema)
+ projected_missing_fields = {}
for field_id in project_schema_diff:
for partition_field in partition_spec.fields_by_source_id(field_id):
if isinstance(partition_field.transform, IdentityTransform):
- accessor = accessors.get(partition_field.field_id)
-
- if accessor is None:
- continue
-
- # The partition field may not exist in the partition record of the data file.
- # This can happen when new partition fields are introduced after the file was written.
- try:
- if partition_value := accessor.get(file.partition):
- projected_missing_fields[partition_field.name] = partition_value
- except IndexError:
- continue
+ if partition_value := accessors[partition_field.field_id].get(file.partition):
+ projected_missing_fields[field_id] = partition_value
Review Comment:
good catch, i see its fixed in https://github.com/apache/iceberg-python/pull/2293/files#diff-8d5e63f2a87ead8cebe2fd8ac5dcf2198d229f01e16bb9e06e21f7277c328abdR1672
now im worried about all the other places we use `.spec()`
--
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] Convert `_get_column_projection_values` to use Field-IDs [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on PR #2293:
URL: https://github.com/apache/iceberg-python/pull/2293#issuecomment-3164393862
I've created the follow-up PR here: https://github.com/apache/iceberg-python/pull/2295
--
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