[bug] Schema validation should reject field names that are invalid Avro identifiers

[nvartolomei]

Apache Iceberg version

None

Please describe the bug 🐞

Example schema:

schema = Schema(
    NestedField(id=1, name="😎", field_type=StringType(), required=False),
)

partition_spec = PartitionSpec(
    PartitionField(
        source_id=1,
        field_id=1001,
        transform=IdentityTransform(),
        name="😎",)
)

Write some data then try to read it with DuckDB or simply:

avrocat /home/nv/src/pyiceberg-example/warehouse/default.db/nested_table/metadata/afc5e55c-6dd2-4875-841c-410108fccf8e-m0.avro | jq .

Error opening /home/nv/src/pyiceberg-example/warehouse/default.db/nested_table/metadata/afc5e55c-6dd2-4875-841c-410108fccf8e-m0.avro:
  Cannot parse file header: Invalid Avro identifier

Willingness to contribute

• I can contribute a fix for this bug independently
• I would be willing to contribute a fix for this bug with guidance from the Iceberg community
• I cannot contribute a fix for this bug at this time

[kevinjqliu]

😎😎😎 Thanks for reporting this issue!

It looks like this is due to the avro naming convention

https://avro.apache.org/docs/1.11.1/specification/#names

The name portion of the fullname of named types, record field names, and enum symbols must:

start with [A-Za-z_]
subsequently contain only [A-Za-z0-9_]

so 😎 is not a valid field name.

We should validate the avro naming convention

[kevinjqliu]

we already have this helper function

iceberg-python/pyiceberg/schema.py

Lines 1364 to 1374 in 89e71c3

	def _valid_avro_name(name: str) -> bool:
	if not len(name):
	raise ValueError("Can not validate empty avro name")
	first = name[0]
	if not (first.isalpha() or first == "_"):
	return False
	for character in name[1:]:
	if not (character.isalnum() or character == "_"):
	return False
	return True

[kris-gaudel]

Could I be assigned this issue please

[kevinjqliu]

@kris-gaudel done! looking forward to the PR

[kris-gaudel]

I see there's a function to sanitize field names called sanitize_column_names, will this function be deprecated if the new behavior is to reject field names that don't already fit the Avro standard? If so should I remove this test?

I was thinking instead of rejecting the name, we could sanitize it in the constructor

iceberg-python/tests/test_schema.py

Lines 440 to 441 in e937f6a

	def test_sanitize() -> None:
	before_sanitized = Schema(

[nvartolomei]

@kevinjqliu if you’ll use this sanitization function on emojis, in particular on a 4 byte one, the Python and Java implementation will encode the field names differently. That’s because Python encodes the whole utf-8 point at a time where’s Java will encode it as 2 2-byte chars (it uses utf-16 internally so 16 byte chars). Can this break anything?

For 😎:

• Java encodes one UTF-16 char at a time: _xD83D_xDE0E.
• Python encodes 4 bytes at a time: _x1F60E.

I’m not working on a PR for this. Discovered this while investigating the behaviour for my custom iceberg implementation.

[kris-gaudel]

@kevinjqliu Would be great to hear your thoughts on this

[kevinjqliu]

Thanks for the note @nvartolomei, thats a great point.

i found the answer from the java reference implementation
https://github.com/apache/iceberg/blob/85cc58aa8acda999926809b3c67bbc3452689490/core/src/main/java/org/apache/iceberg/avro/TypeToSchema.java#L116-L129

we should match the behavior for validAvroName, sanitize, and ICEBERG_FIELD_NAME_PROP

this is also used on the read side
https://github.com/apache/iceberg/blob/85cc58aa8acda999926809b3c67bbc3452689490/core/src/main/java/org/apache/iceberg/avro/BuildAvroProjection.java#L98-L122

And these are great tests to include
https://github.com/apache/iceberg/blob/85cc58aa8acda999926809b3c67bbc3452689490/core/src/test/java/org/apache/iceberg/avro/TestSchemaConversions.java#L334-L358

[kris-gaudel]

I'll close my other PR and raise another one implementing this approach

[kevinjqliu]

thanks @kris-gaudel let me know if theres anything i can help with.

#83 was the previous fix for the avro column sanitization