Skip to content

Commit 772b55d

Browse files
hzmshclaude
andcommitted
fix(avro): keep the table readable after dropping an old partition column
After a partition field is removed and its source column is dropped, the old spec's partition field has the unknown type. Rewriting its manifest wrapped the Avro "null" type in another null union, which is invalid Avro and made the table unreadable. Write unknown fields as a plain "null", like Java's AvroSchemaUtil.toOption. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 parent b32392d commit 772b55d

4 files changed

Lines changed: 49 additions & 3 deletions

File tree

‎pyiceberg/avro/resolver.py‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -147,7 +147,7 @@ def struct(self, struct: StructType, field_results: list[Writer]) -> Writer:
147147
return StructWriter(tuple((pos, result) for pos, result in enumerate(field_results)))
148148

149149
def field(self, field: NestedField, field_result: Writer) -> Writer:
150-
return field_result if field.required else OptionWriter(field_result)
150+
return field_result if field.required or isinstance(field.field_type, UnknownType) else OptionWriter(field_result)
151151

152152
def list(self, list_type: ListType, element_result: Writer) -> Writer:
153153
return ListWriter(element_result)
@@ -302,7 +302,9 @@ def struct(self, file_schema: StructType, record_struct: IcebergType | None, fil
302302
return StructWriter(field_writers=tuple(results))
303303

304304
def field(self, file_field: NestedField, record_type: IcebergType | None, field_writer: Writer) -> Writer:
305-
return field_writer if file_field.required else OptionWriter(field_writer)
305+
return (
306+
field_writer if file_field.required or isinstance(file_field.field_type, UnknownType) else OptionWriter(field_writer)
307+
)
306308

307309
def list(self, file_list_type: ListType, file_list: IcebergType | None, element_writer: Writer) -> Writer:
308310
return ListWriter(element_writer if file_list_type.element_required else OptionWriter(element_writer))

‎pyiceberg/utils/schema_conversion.py‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -534,7 +534,8 @@ def field(self, field: NestedField, field_result: AvroType) -> AvroType:
534534
result = {
535535
"name": sanitized_name,
536536
FIELD_ID_PROP: field.field_id,
537-
"type": field_result if field.required else ["null", field_result],
537+
# Unknown maps to Avro "null", and a union cannot contain "null" twice
538+
"type": field_result if field.required or isinstance(field.field_type, UnknownType) else ["null", field_result],
538539
}
539540

540541
if original_name != sanitized_name:

‎tests/avro/test_file.py‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,7 @@
5454
TimestampType,
5555
TimestamptzType,
5656
TimeType,
57+
UnknownType,
5758
UUIDType,
5859
)
5960
from pyiceberg.utils.schema_conversion import AvroSchemaConversion
@@ -544,3 +545,23 @@ def test_avro_output_file_tell_includes_buffered_records() -> None:
544545
out.write_block([manifest_entry(0)])
545546

546547
assert out.tell() > after_header
548+
549+
550+
def test_write_optional_unknown_type() -> None:
551+
# A partition field whose source column was dropped has the unknown type
552+
schema = Schema(
553+
NestedField(field_id=1, name="before", field_type=StringType(), required=False),
554+
NestedField(field_id=2, name="dropped", field_type=UnknownType(), required=False),
555+
NestedField(field_id=3, name="after", field_type=LongType(), required=True),
556+
)
557+
558+
with TemporaryDirectory() as tmpdir:
559+
tmp_avro_file = tmpdir + "/unknown.avro"
560+
with avro.AvroOutputFile[Record](PyArrowFileIO().new_output(tmp_avro_file), schema, "unknown") as out:
561+
out.write_block([Record("hello", None, 42)])
562+
563+
with avro.AvroFile[Record](PyArrowFileIO().new_input(tmp_avro_file), schema, {-1: Record}) as avro_reader:
564+
assert list(avro_reader) == [Record("hello", None, 42)]
565+
566+
with open(tmp_avro_file, "rb") as fo:
567+
assert list(reader(fo=fo)) == [{"before": "hello", "dropped": None, "after": 42}]

‎tests/catalog/test_catalog_behaviors.py‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -848,6 +848,28 @@ def test_write_and_evolve(catalog: Catalog, format_version: int) -> None:
848848
txn.delete("foo = 'a'")
849849

850850

851+
@pytest.mark.parametrize("format_version", [1, 2])
852+
def test_delete_after_dropping_old_partition_source_column(catalog: Catalog, format_version: int) -> None:
853+
identifier = f"default.delete_after_dropping_old_partition_source_v{format_version}"
854+
catalog.create_namespace_if_not_exists("default")
855+
856+
pa_table = pa.Table.from_pydict({"id": [1, 2], "category": ["a", "b"]})
857+
tbl = catalog.create_table(identifier=identifier, schema=pa_table.schema, properties={"format-version": str(format_version)})
858+
with tbl.update_spec() as spec_update:
859+
spec_update.add_identity("category")
860+
tbl.append(pa_table)
861+
862+
with tbl.update_spec() as spec_update:
863+
spec_update.remove_field("category")
864+
with tbl.update_schema() as schema_update:
865+
schema_update.delete_column("category")
866+
867+
# Rewrites the manifest of the old spec, whose partition source column is gone
868+
tbl.delete("id == 1")
869+
870+
assert catalog.load_table(identifier).scan().to_arrow()["id"].to_pylist() == [2]
871+
872+
851873
# Merge manifests
852874
@pytest.mark.parametrize("format_version", [1, 2])
853875
def test_merge_manifests_local_file_system(catalog: Catalog, arrow_table_with_null: pa.Table, format_version: int) -> None:

0 commit comments

Comments
 (0)