Skip to content

Avro: define named types once and read references to them - #4119

Open
ozenyunus35 wants to merge 2 commits into
apache:mainfrom
ozenyunus35:fix/avro-named-type-references
Open

ozenyunus35 wants to merge 2 commits into
apache:mainfrom
ozenyunus35:fix/avro-named-type-references

Conversation

@ozenyunus35

Copy link
Copy Markdown

Closes #4116

Rationale for this change

As described in the issue, an Avro named type (decimal_P_S, fixed_L, uuid_fixed) may only be defined once per schema; later uses must refer to it by name. PyIceberg couldn't read the reference form (TypeError: Type not recognized: decimal_10_2), so it failed on manifests written by Java and iceberg-rust, and it wrote the full definition every time, which strict readers reject. This follows the approach in the issue: lenient on read, strict on write.

  • Read: AvroSchemaConversion remembers each named type (record, enum, fixed) it converts, and resolves a later string type with that name to the same Iceberg type. A repeated full definition is still accepted, so existing manifests keep working. The map is reset at the start of avro_to_iceberg.
  • Write: ConvertSchemaToAvro emits the definition of a fixed, decimal or uuid type the first time its name comes up and the bare name after that. The visitor walks fields in the order they are serialized, so the definition always comes before its references.

Are these changes tested?

Yes, in the existing test files:

  • tests/utils/test_schema_conversion.py: reading the reference form (the repro from the issue, plus fixed_3 and uuid_fixed), reading the repeated form, an unknown name still raising TypeError, and writing a schema with repeated decimal, fixed (one inside a struct) and uuid types: the first use is a definition, later uses are names, a different decimal gets its own definition, and the result reads back to the same schema.
  • tests/avro/test_file.py: test_repeated_named_types_round_trip writes a file with two decimal(10,2) and two uuid fields through AvroOutputFile and reads it back with PyIceberg and with fastavro. Without the change, fastavro refuses the file with a SchemaParseException for the redefined name.

The new tests fail without the change. tests/utils/test_schema_conversion.py, tests/avro, tests/test_avro_sanitization.py and tests/utils/test_manifest.py pass. In a full -m unit run, the same 50 tests fail before and after the change in my environment (optional extras I didn't install, such as the Entra and Hive dependencies). ruff 0.14.3 and mypy 1.18.2 (the pre-commit versions) are clean.

Are there any user-facing changes?

Manifests and manifest lists with repeated named types can now be read when written in the reference form, and PyIceberg now writes the reference form.

I used an AI assistant to help draft parts of the code and tests; I reviewed the changes and ran the tests myself.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Avro: can't read named type references, and writes repeated definitions (e.g. multiple decimal partition fields)

1 participant