laskoviymishka commented on code in PR #3070:
URL: https://github.com/apache/iceberg-rust/pull/3070#discussion_r4148569157
##########
crates/iceberg/src/spec/schema/mod.rs:
##########
@@ -342,13 +346,36 @@ 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))
+ .and_then(|id| id.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>> {
+ match self.lowercase_name_to_id.get(&field_name.to_lowercase()) {
+ Some(Some(id)) => Ok(self.field_by_id(*id)),
+ Some(None) => Err(Error::new(
Review Comment:
Small idiom thing, independent of whether you touch the message above:
`invalid_data!` is already imported and used everywhere else in this file for
`DataInvalid`, so I'd use it here instead of the raw
`Error::new(ErrorKind::DataInvalid, …)` — otherwise a code-search sweep over
`invalid_data!` sites quietly misses this one.
##########
crates/iceberg/src/spec/schema/mod.rs:
##########
@@ -342,13 +346,36 @@ 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))
+ .and_then(|id| id.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>> {
+ match self.lowercase_name_to_id.get(&field_name.to_lowercase()) {
+ Some(Some(id)) => Ok(self.field_by_id(*id)),
+ Some(None) => Err(Error::new(
+ ErrorKind::DataInvalid,
+ format!("Multiple fields match {field_name}
case-insensitively"),
Review Comment:
The colliding-names message is the one thing still open from last round — it
names `Id` but not what it collided with. You've kept it naming just the lookup
key, which is a perfectly fine call and I'm not going to hold on it.
If you do decide you want the Java-style "id and ID collide", it's cheap on
the error path and doesn't touch the stored map:
```rust
Some(None) => {
let lower = field_name.to_lowercase();
let colliders: Vec<&str> = self
.name_to_id
.keys()
.filter(|k| k.to_lowercase() == lower)
.map(String::as_str)
.collect();
Err(invalid_data!(
"Multiple fields match {field_name} case-insensitively:
{colliders:?}"
))
}
```
If you take it, the three tests asserting the full `Multiple fields match Id
case-insensitively` substring want loosening to the kind plus a stable
fragment, so a wording tweak doesn't break all three at once.
##########
crates/iceberg/src/spec/schema/mod.rs:
##########
@@ -342,13 +346,36 @@ 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))
+ .and_then(|id| id.and_then(|id| self.field_by_id(id)))
Review Comment:
Readability: the double `id` shadow leans on `Option<i32>: Copy` to deref
invisibly. `.copied().flatten()` reads clearer and drops the shadow:
```rust
self.lowercase_name_to_id
.get(&field_name.to_lowercase())
.copied()
.flatten()
.and_then(|id| self.field_by_id(id))
```
--
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]
