This is an automated email from the ASF dual-hosted git repository. github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-3070-e42840bc98cbf91a67c014162ed68036a8452d7a in repository https://gitbox.apache.org/repos/asf/iceberg-rust.git
commit d63e8610f2152158882f8773f3745f3f32752912 Author: Manu Zhang <[email protected]> AuthorDate: Sun Oct 11 13:53:01 2026 +0000 fix(scan): reject ambiguous case-insensitive fields (#3070) * fix(scan): reject ambiguous case-insensitive fields Co-authored-by: Codex <[email protected]> * fix(schema): clarify ambiguous case-insensitive lookup Expose the checked lookup, document collision semantics, and cover the public fallback. Simplify the binding and scan callers. Co-authored-by: Codex <[email protected]> * fix(schema): improve case-insensitive collision diagnostics List sorted colliding field names with invalid_data!, simplify optional lookup, and loosen error assertions while checking the reported colliders. Co-authored-by: Codex <[email protected]> --------- Co-authored-by: Codex <[email protected]> --- crates/iceberg/public-api.txt | 1 + crates/iceberg/src/expr/term.rs | 19 +++++++- crates/iceberg/src/scan/mod.rs | 68 ++++++++++++++++++++++++--- crates/iceberg/src/spec/schema/mod.rs | 86 ++++++++++++++++++++++++++++++++--- 4 files changed, 160 insertions(+), 14 deletions(-) diff --git a/crates/iceberg/public-api.txt b/crates/iceberg/public-api.txt index 8a34bbf7b..466614757 100644 --- a/crates/iceberg/public-api.txt +++ b/crates/iceberg/public-api.txt @@ -2474,6 +2474,7 @@ pub fn iceberg::spec::Schema::field_by_alias(&self, alias: &str) -> core::option pub fn iceberg::spec::Schema::field_by_id(&self, field_id: i32) -> core::option::Option<&iceberg::spec::NestedFieldRef> pub fn iceberg::spec::Schema::field_by_name(&self, field_name: &str) -> core::option::Option<&iceberg::spec::NestedFieldRef> pub fn iceberg::spec::Schema::field_by_name_case_insensitive(&self, field_name: &str) -> core::option::Option<&iceberg::spec::NestedFieldRef> +pub fn iceberg::spec::Schema::field_by_name_case_insensitive_checked(&self, field_name: &str) -> iceberg::Result<core::option::Option<&iceberg::spec::NestedFieldRef>> pub fn iceberg::spec::Schema::field_id_by_name(&self, name: &str) -> core::option::Option<i32> pub fn iceberg::spec::Schema::field_id_to_fields(&self) -> &std::collections::hash::map::HashMap<i32, iceberg::spec::NestedFieldRef> pub fn iceberg::spec::Schema::field_id_to_name_map(&self) -> &std::collections::hash::map::HashMap<i32, alloc::string::String> diff --git a/crates/iceberg/src/expr/term.rs b/crates/iceberg/src/expr/term.rs index 4a5aedf74..7c5cc0891 100644 --- a/crates/iceberg/src/expr/term.rs +++ b/crates/iceberg/src/expr/term.rs @@ -313,7 +313,7 @@ impl Bind for Reference { let field = if case_sensitive { schema.field_by_name(&self.name) } else { - schema.field_by_name_case_insensitive(&self.name) + schema.field_by_name_case_insensitive_checked(&self.name)? }; let field = @@ -428,6 +428,23 @@ mod tests { assert_eq!(expected_ref, reference); } + #[test] + fn test_bind_reference_case_insensitive_ambiguous() { + let schema = Arc::new( + Schema::builder() + .with_fields(vec![ + NestedField::optional(1, "id", PrimitiveType::Int.into()).into(), + NestedField::optional(2, "ID", PrimitiveType::Int.into()).into(), + ]) + .build() + .unwrap(), + ); + + let error = Reference::new("Id").bind(schema, false).unwrap_err(); + assert_eq!(error.kind(), crate::ErrorKind::DataInvalid); + assert!(error.message().contains("case-insensitively"), "{error}"); + } + #[test] fn test_bind_reference_failure() { let schema = table_schema_simple(); diff --git a/crates/iceberg/src/scan/mod.rs b/crates/iceberg/src/scan/mod.rs index 0c241125d..7e81900a5 100644 --- a/crates/iceberg/src/scan/mod.rs +++ b/crates/iceberg/src/scan/mod.rs @@ -53,13 +53,16 @@ use crate::{Error, ErrorKind, Result}; pub type ArrowRecordBatchStream = BoxStream<'static, Result<RecordBatch>>; /// Resolves a column name to its field ID, honouring the scan's case sensitivity. -fn resolve_field_id(schema: &Schema, column_name: &str, case_sensitive: bool) -> Option<i32> { +fn resolve_field_id( + schema: &Schema, + column_name: &str, + case_sensitive: bool, +) -> Result<Option<i32>> { if case_sensitive { - schema.field_id_by_name(column_name) + Ok(schema.field_id_by_name(column_name)) } else { - schema - .field_by_name_case_insensitive(column_name) - .map(|field| field.id) + let field = schema.field_by_name_case_insensitive_checked(column_name)?; + Ok(field.map(|field| field.id)) } } @@ -79,7 +82,7 @@ fn collect_scan_field_ids( return get_metadata_field_id(column_name); } - let field_id = resolve_field_id(schema, column_name, case_sensitive).ok_or_else(|| { + let field_id = resolve_field_id(schema, column_name, case_sensitive)?.ok_or_else(|| { invalid_data!("Column {column_name} not found in table. Schema: {schema}") })?; @@ -692,6 +695,7 @@ mod tests { use futures::{TryStreamExt, stream}; use uuid::Uuid; + use super::resolve_field_id; use crate::arrow::ArrowReaderBuilder; use crate::expr::{BoundPredicate, Reference}; use crate::io::FileIO; @@ -761,6 +765,58 @@ mod tests { ); } + #[test] + fn test_resolve_field_id_case_insensitive_rejects_ambiguous_name() { + let schema = Schema::builder() + .with_fields(vec![ + NestedField::optional(1, "id", PrimitiveType::Int.into()).into(), + NestedField::optional(2, "ID", PrimitiveType::Int.into()).into(), + NestedField::optional(3, "data", PrimitiveType::Int.into()).into(), + ]) + .build() + .unwrap(); + + assert_eq!(resolve_field_id(&schema, "id", true).unwrap(), Some(1)); + assert_eq!(resolve_field_id(&schema, "DATA", false).unwrap(), Some(3)); + let error = resolve_field_id(&schema, "Id", false).unwrap_err(); + assert_eq!(error.kind(), ErrorKind::DataInvalid); + assert!(error.message().contains("case-insensitively"), "{error}"); + } + + #[test] + fn test_case_insensitive_scan_with_ambiguous_schema() { + let fixture = TableTestFixture::new(); + let mut metadata = fixture.table.metadata().clone(); + let schema = Schema::builder() + .with_schema_id(metadata.current_schema_id()) + .with_fields(vec![ + NestedField::optional(1, "id", PrimitiveType::Int.into()).into(), + NestedField::optional(2, "ID", PrimitiveType::Int.into()).into(), + NestedField::optional(3, "data", PrimitiveType::Int.into()).into(), + ]) + .build() + .unwrap(); + metadata + .schemas + .insert(metadata.current_schema_id(), Arc::new(schema)); + let table = fixture.table.with_metadata(Arc::new(metadata)); + + table + .scan() + .with_case_sensitive(false) + .select(["DATA"]) + .build() + .unwrap(); + let error = table + .scan() + .with_case_sensitive(false) + .select(["Id"]) + .build() + .unwrap_err(); + assert_eq!(error.kind(), ErrorKind::DataInvalid); + assert!(error.message().contains("case-insensitively"), "{error}"); + } + #[tokio::test] async fn test_case_insensitive_scan_resolves_mismatched_column_case() { let mut fixture = TableTestFixture::new(); diff --git a/crates/iceberg/src/spec/schema/mod.rs b/crates/iceberg/src/spec/schema/mod.rs index c6dcf0707..67924dfe8 100644 --- a/crates/iceberg/src/spec/schema/mod.rs +++ b/crates/iceberg/src/spec/schema/mod.rs @@ -71,7 +71,8 @@ pub struct Schema { id_to_field: HashMap<i32, NestedFieldRef>, name_to_id: HashMap<String, i32>, - lowercase_name_to_id: HashMap<String, i32>, + // `None` marks a lower-cased name that resolves to more than one field. + lowercase_name_to_id: HashMap<String, Option<i32>>, id_to_name: HashMap<i32, String>, field_id_to_accessor: HashMap<i32, Arc<StructAccessor>>, @@ -150,10 +151,13 @@ impl SchemaBuilder { index.indexes() }; - let lowercase_name_to_id = name_to_id - .iter() - .map(|(k, v)| (k.to_lowercase(), *v)) - .collect(); + let mut lowercase_name_to_id = HashMap::new(); + for (name, field_id) in &name_to_id { + lowercase_name_to_id + .entry(name.to_lowercase()) + .and_modify(|existing| *existing = None) + .or_insert(Some(*field_id)); + } let highest_field_id = id_to_field.keys().max().cloned().unwrap_or(0); @@ -342,13 +346,47 @@ impl Schema { .and_then(|id| self.field_by_id(*id)) } - /// Get field by field name, but in case-insensitive way. + /// Get field by field name in a case-insensitive way. /// /// Both full name and short name could work here. + /// Returns `None` if the name is missing or matches multiple fields case-insensitively. + /// Use [`Self::field_by_name_case_insensitive_checked`] to distinguish those cases. pub fn field_by_name_case_insensitive(&self, field_name: &str) -> Option<&NestedFieldRef> { self.lowercase_name_to_id .get(&field_name.to_lowercase()) - .and_then(|id| self.field_by_id(*id)) + .copied() + .flatten() + .and_then(|id| self.field_by_id(id)) + } + + /// Get field by field name in a case-insensitive way, reporting ambiguous names. + /// + /// Both full name and short name could work here. Returns `Ok(None)` if the name is + /// missing, or an error if it matches multiple fields case-insensitively. A collision + /// on another name does not prevent this name from resolving. This differs from Iceberg + /// Java, which rejects all case-insensitive lookups after a collision, and PyIceberg, + /// which silently keeps one of the colliding fields. + pub fn field_by_name_case_insensitive_checked( + &self, + field_name: &str, + ) -> Result<Option<&NestedFieldRef>> { + let lowercase_name = field_name.to_lowercase(); + match self.lowercase_name_to_id.get(&lowercase_name) { + Some(Some(id)) => Ok(self.field_by_id(*id)), + Some(None) => { + let mut colliders: Vec<&str> = self + .name_to_id + .keys() + .filter(|name| name.to_lowercase() == lowercase_name) + .map(String::as_str) + .collect(); + colliders.sort_unstable(); + Err(invalid_data!( + "Multiple fields match {field_name} case-insensitively: {colliders:?}" + )) + } + None => Ok(None), + } } /// Get field by alias. @@ -973,6 +1011,40 @@ table { } } + #[test] + fn test_schema_field_by_name_case_insensitive_checked() { + let schema = Schema::builder() + .with_fields(vec![ + NestedField::optional(1, "id", Primitive(PrimitiveType::Int)).into(), + NestedField::optional(2, "ID", Primitive(PrimitiveType::Int)).into(), + NestedField::optional(3, "data", Primitive(PrimitiveType::String)).into(), + ]) + .build() + .expect("case-insensitive name ambiguity is valid in a schema"); + + assert_eq!( + schema + .field_by_name_case_insensitive_checked("DATA") + .unwrap() + .map(|field| field.id), + Some(3) + ); + assert!( + schema + .field_by_name_case_insensitive_checked("missing") + .unwrap() + .is_none() + ); + + let error = schema + .field_by_name_case_insensitive_checked("Id") + .unwrap_err(); + assert_eq!(error.kind(), crate::ErrorKind::DataInvalid); + assert!(error.message().contains("case-insensitively"), "{error}"); + assert!(error.message().contains("[\"ID\", \"id\"]"), "{error}"); + assert!(schema.field_by_name_case_insensitive("Id").is_none()); + } + #[test] fn test_schema_find_column_name() { let expected_column_name = HashMap::from([
