SreeramGarlapati commented on code in PR #2540:
URL: https://github.com/apache/iceberg-rust/pull/2540#discussion_r3449730044
##########
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:
good call. i'd keep the local check regardless though — `is_valid_avro_name`
is the exact Avro grammar `[A-Za-z_][A-Za-z0-9_]*` in ~6 lines with no coupling
to avro-rs internals, and the sanitize half is the iceberg `iceberg-field-name`
convention which avro-rs doesn't cover anyway. happy to file an upstream issue
to expose `validate_record_field_name` as good-citizen cleanup and link it here
— lmk if you'd like that.
##########
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:
your conclusion holds — representational parity with Java isn't needed — but
let me correct the reasoning (incl. something I'd have gotten wrong).
this impl is deliberately **not** matching Java's `isDigit`. Java's
`validAvroName`/`sanitize` use the unicode
`Character.isLetter`/`isLetterOrDigit`/`isDigit`, so Java keeps non-ASCII
letters/digits as-is (`café`, leading `٠` → `_٠`, …) — which actually violates
Avro's ASCII-only name grammar `[A-Za-z_][A-Za-z0-9_]*`. matching Java's
`isDigit` here would re-introduce the exact invalid-Avro bug this PR fixes. so
we stay ASCII-strict: anything outside ASCII alnum/`_` is escaped `_x<HEX>`,
output is always spec-valid, original preserved in `iceberg-field-name`.
correction to the framing — it's **not** "both restore from the map". I
checked the Java source: `iceberg-field-name` is **write-only** in Java
(written by `TypeToSchema` + `BuildAvroProjection`; no read site). Java's
`SchemaToType` just uses `field.name()`, and the authoritative name is
recovered by field-id projection against the table schema. so correctness
doesn't depend on the escape representation either way: within iceberg-rust the
round-trip is lossless because we write *and* read the prop; cross-engine,
names resolve by field-id. our read path honoring the prop is actually a touch
more faithful than Java's `SchemaToType` on the standalone conversion path.
net: not changing the digit logic. I did fix the doc, which wrongly claimed
it "matches Java semantics" — it follows Java's escape scheme but is
intentionally ASCII-stricter.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
