fix: sanitize Avro field names on write, respect iceberg-field-name on read#2540
fix: sanitize Avro field names on write, respect iceberg-field-name on read#2540SreeramGarlapati wants to merge 2 commits into
Conversation
…ld-name Iceberg field names can be arbitrary (leading digits, dots, spaces, etc.) but Avro requires names to match [A-Za-z_][A-Za-z0-9_]*. Java handles this by sanitizing invalid names on write and storing the original in an "iceberg-field-name" custom property, then checking that property on read. iceberg-rust was writing unsanitized names directly, which causes Avro validation failures (or produces files unreadable by strict Avro parsers) when field names don't conform to Avro's naming rules. This adds: - sanitize_avro_name(): matches Java's AvroSchemaUtil.sanitize() logic (prefix _ for leading digits, _x<HEX> for special chars) - Write path: sanitizes the field name and stores the original in iceberg-field-name when sanitization was needed - Read path: checks iceberg-field-name property first, falls back to the Avro field name Closes apache#2535 Co-authored-by: rawataaryan9 <rawataaryan9@users.noreply.github.com>
…tion Adds test cases for: - Non-ASCII BMP characters (U+00E9, U+4E2D) - Supplementary characters (surrogate pair handling, matching Java's UTF-16) - Empty string edge case - Read-path: iceberg-field-name property resolution from Java-written schemas - Verify iceberg-field-name property is set on dotted field names Co-authored-by: rawataaryan9 <rawataaryan9@users.noreply.github.com>
fe8c3ff to
12ba3b5
Compare
xanderbailey
left a comment
There was a problem hiding this comment.
Nice PR, thanks for the contribution, left a couple of questions
| // This const may better to maintain in avro-rs. | ||
| const LOGICAL_TYPE: &str = "logicalType"; | ||
|
|
||
| fn is_valid_avro_name(name: &str) -> bool { |
There was a problem hiding this comment.
It would be nice if we could use something from https://github.com/apache/avro-rs/blob/4edb1ce1ae1ab5bd3fafb08ca3f622946c01c9fd/avro/src/validator.rs#L4 but it looks like validate_record_field_name is pub(crate). Is it work filing an issue upstream to see if we could expose something that would allow us to validate against their implementation?
| fn is_ascii_digit_u16(c: u16) -> bool { | ||
| matches!(c, 0x30..=0x39) | ||
| } |
There was a problem hiding this comment.
/**
* Determines if the specified character is a digit.
* <p>
* A character is a digit if its general category type, provided
* by {@code Character.getType(ch)}, is
* {@code DECIMAL_DIGIT_NUMBER}.
* <p>
* Some Unicode character ranges that contain digits:
* <ul>
* <li>{@code '\u005Cu0030'} through {@code '\u005Cu0039'},
* ISO-LATIN-1 digits ({@code '0'} through {@code '9'})
* <li>{@code '\u005Cu0660'} through {@code '\u005Cu0669'},
* Arabic-Indic digits
* <li>{@code '\u005Cu06F0'} through {@code '\u005Cu06F9'},
* Extended Arabic-Indic digits
* <li>{@code '\u005Cu0966'} through {@code '\u005Cu096F'},
* Devanagari digits
* <li>{@code '\u005CuFF10'} through {@code '\u005CuFF19'},
* Fullwidth digits
* </ul>
*
* Many other character ranges contain digits as well.
*
* <p><b>Note:</b> This method cannot handle <a
* href="#supplementary"> supplementary characters</a>. To support
* all Unicode characters, including supplementary characters, use
* the {@link #isDigit(int)} method.
*
* @param ch the character to be tested.
* @return {@code true} if the character is a digit;
* {@code false} otherwise.
* @see Character#digit(char, int)
* @see Character#forDigit(int, int)
* @see Character#getType(char)
*/
public static boolean isDigit(char ch) {
return isDigit((int)ch);
}https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/avro/AvroSchemaUtil.java#L551
Java's isDigit() actually covers more than just ascii digits. This is a pretty niche edge-case.
Trying to reason about this in my head and I don't think it matters for interop that the representations are identical because both will correctly underscore the field and restore it from the map? Can you check my reasoning here?
Summary
Iceberg field names can be anything (
123column,field.with.dots, etc.) but Avro requires names to match[A-Za-z_][A-Za-z0-9_]*. Java handles this with a sanitize-on-write + restore-on-read protocol using theiceberg-field-namecustom Avro property. iceberg-rust was doing neither — writing invalid names directly and ignoring the property on read.This causes two interop failures:
Changes
Write path (Iceberg→Avro conversion in
SchemaToAvroSchema::field):is_valid_avro_name()— checks[A-Za-z_][A-Za-z0-9_]*sanitize_avro_name()— matches Java'sAvroSchemaUtil.sanitize():_(e.g.,123col→_123col)_x<HEX>(e.g.,field.name→field_x2Ename)charAt()behavior for supplementary charsiceberg-field-namepropertyRead path (Avro→Iceberg conversion in
AvroSchemaToSchema::record):iceberg-field-namecustom attribute first, falls back to Avro field nameJava reference
AvroSchemaUtil.sanitize()TypeToSchema.struct()— storesICEBERG_FIELD_NAME_PROPAvroSchemaUtil.ICEBERG_FIELD_NAME_PROPCloses #2535
Test plan
test_is_valid_avro_name— validates detection of invalid namestest_sanitize_avro_name— ASCII edge cases match Java behaviortest_sanitize_avro_name_unicode— BMP and supplementary char handling (surrogate pairs)test_sanitization_round_trip— Iceberg→Avro→Iceberg preserves original namestest_avro_to_iceberg_uses_iceberg_field_name_property— reads Java-written schemas correctly