You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "xanderbailey (via GitHub)" <gi...@apache.org> on 2026/06/01 11:42:09 UTC
Re: [PR] fix: sanitize Avro field names on write, respect iceberg-field-name on read [iceberg-rust]
xanderbailey commented on code in PR #2540:
URL: https://github.com/apache/iceberg-rust/pull/2540#discussion_r3333735640
##########
crates/iceberg/src/avro/schema.rs:
##########
@@ -34,12 +34,83 @@ use crate::{Error, ErrorKind, Result, ensure_data_valid};
const ELEMENT_ID: &str = "element-id";
const FIELD_ID_PROP: &str = "field-id";
+const ICEBERG_FIELD_NAME_PROP: &str = "iceberg-field-name";
const KEY_ID: &str = "key-id";
const VALUE_ID: &str = "value-id";
const MAP_LOGICAL_TYPE: &str = "map";
// This const may better to maintain in avro-rs.
const LOGICAL_TYPE: &str = "logicalType";
+fn is_valid_avro_name(name: &str) -> bool {
Review 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?
##########
crates/iceberg/src/avro/schema.rs:
##########
@@ -34,12 +34,83 @@ use crate::{Error, ErrorKind, Result, ensure_data_valid};
const ELEMENT_ID: &str = "element-id";
const FIELD_ID_PROP: &str = "field-id";
+const ICEBERG_FIELD_NAME_PROP: &str = "iceberg-field-name";
const KEY_ID: &str = "key-id";
const VALUE_ID: &str = "value-id";
const MAP_LOGICAL_TYPE: &str = "map";
// This const may better to maintain in avro-rs.
const LOGICAL_TYPE: &str = "logicalType";
+fn is_valid_avro_name(name: &str) -> bool {
+ let mut chars = name.chars();
+ match chars.next() {
+ None => false,
+ Some(first) => {
+ (first.is_ascii_alphabetic() || first == '_')
+ && chars.all(|c| c.is_ascii_alphanumeric() || c == '_')
+ }
+ }
+}
+
+/// Sanitizes an Iceberg field name to a valid Avro field name.
+///
+/// Matches Java `AvroSchemaUtil.sanitize()` semantics, operating on UTF-16
+/// code units (to match Java's `String.charAt()`). Characters that are not
+/// ASCII letters, ASCII digits, or underscore are escaped as `_x<HEX>` where
+/// HEX is the uppercase hexadecimal representation of the UTF-16 code unit
+/// with no leading zeros.
+///
+/// Special handling for the first character:
+/// - ASCII digit: prefix with `_`, digit is preserved (e.g., `1foo` -> `_1foo`)
+/// - Non-letter, non-underscore: escaped as `_x<HEX>` (e.g., `.foo` -> `_x2Efoo`)
+///
+/// For supplementary characters (above U+FFFF), each surrogate half is escaped
+/// independently (e.g., U+1F600 -> `_xD83D_xDE00`), matching Java's behavior
+/// of iterating over `char` (UTF-16 code unit) values.
+fn sanitize_avro_name(name: &str) -> String {
+ let utf16_units: Vec<u16> = name.encode_utf16().collect();
+ if utf16_units.is_empty() {
+ return String::new();
+ }
+
+ let mut result = String::with_capacity(name.len() + 16);
+
+ let first = utf16_units[0];
+ if is_ascii_alpha_u16(first) || first == b'_' as u16 {
+ result.push(first as u8 as char);
+ } else if is_ascii_digit_u16(first) {
+ result.push('_');
+ result.push(first as u8 as char);
+ } else {
+ result.push_str(&format!("_x{:X}", first));
+ }
+
+ for &unit in &utf16_units[1..] {
+ if is_ascii_alphanum_u16(unit) || unit == b'_' as u16 {
+ result.push(unit as u8 as char);
+ } else {
+ result.push_str(&format!("_x{:X}", unit));
+ }
+ }
+
+ result
+}
+
+#[inline]
+fn is_ascii_alpha_u16(c: u16) -> bool {
+ matches!(c, 0x41..=0x5A | 0x61..=0x7A)
+}
+
+#[inline]
+fn is_ascii_digit_u16(c: u16) -> bool {
+ matches!(c, 0x30..=0x39)
+}
Review Comment:
```java
/**
* 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?
--
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