laskoviymishka commented on code in PR #3070:
URL: https://github.com/apache/iceberg-rust/pull/3070#discussion_r4087492616
##########
crates/iceberg/src/spec/schema/mod.rs:
##########
@@ -348,7 +352,21 @@ impl Schema {
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:
I'd promote `_checked` to `pub` and point this doc comment at it, or at
minimum document that `None` now means "missing or ambiguous" — because this is
stable public surface (it's in `public-api.txt`) and the contract just moved
silently.
Before, a caller always got *some* field back for a name with case-variants;
now an ambiguous name returns `None`, identical to "field doesn't exist," and
the only variant that can tell the two apart is `pub(crate)`. So external
callers both lose the old behavior and can't reach the new one. The doc still
just says "in a case-insensitive way," which no longer tells the whole story.
##########
crates/iceberg/src/spec/schema/mod.rs:
##########
@@ -973,6 +991,38 @@ table {
}
}
+ #[test]
+ fn test_schema_field_by_name_case_insensitive_checked() {
Review Comment:
While we're adding `_checked` coverage: nothing pins the new behavior of the
*public* `field_by_name_case_insensitive` on a colliding schema. A one-liner
asserting it returns `None` for `Id` would make the silent contract change
visible if someone touches this later.
##########
crates/iceberg/src/spec/schema/mod.rs:
##########
@@ -348,7 +352,21 @@ impl Schema {
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)))
+ }
+
+ pub(crate) 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!("Field name {field_name} is ambiguous when case
sensitivity is disabled"),
Review Comment:
The message can only name the lookup key, not what it collided with, because
the sentinel drops the IDs back at build time (`*existing = None`). Java's
reads "id and ID collide," which is a lot more actionable against a wide schema.
If we want that, the map value could hold the colliders (a small `Vec<i32>`
or a two-ID tuple) instead of `Option<i32>` — the extra alloc only happens on
the rare collision. If we'd rather keep the sentinel, I'd at least reword:
"when case sensitivity is disabled" reads a little oddly, something like
`"multiple fields match {field_name} case-insensitively"` is clearer. wdyt?
##########
crates/iceberg/src/expr/term.rs:
##########
@@ -311,10 +311,10 @@ impl Bind for Reference {
fn bind(&self, schema: SchemaRef, case_sensitive: bool) ->
crate::Result<Self::Bound> {
let field = if case_sensitive {
- schema.field_by_name(&self.name)
+ Ok(schema.field_by_name(&self.name))
Review Comment:
The `Ok(...)` wrapper on the case-sensitive arm only exists so the trailing
`}?` typechecks across both arms. Moving the `?` into the else arm reads
cleaner and makes it obvious only that branch returns early:
```rust
let field = if case_sensitive {
schema.field_by_name(&self.name)
} else {
schema.field_by_name_case_insensitive_checked(&self.name)?
};
```
##########
crates/iceberg/src/spec/schema/mod.rs:
##########
@@ -348,7 +352,21 @@ impl Schema {
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)))
+ }
+
+ pub(crate) fn field_by_name_case_insensitive_checked(
Review Comment:
`_checked` has no doc comment, and this is the spot to note that the
per-name check is an intentional Rust-only refinement. Java's index is
all-or-nothing — any collision in the schema poisons *every* case-insensitive
lookup, so even unambiguous `data` throws — and PyIceberg silently keeps the
last collider. Same table + column can therefore succeed here, error in Spark,
and pick-one in PyIceberg. Ours is arguably the nicest of the three, so I
wouldn't change it; I'd just document the divergence and spell out what
"ambiguous" means while adding the doc.
##########
crates/iceberg/src/scan/mod.rs:
##########
@@ -52,13 +52,17 @@ 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)
+ Ok(schema
+ .field_by_name_case_insensitive_checked(column_name)?
Review Comment:
Same shape as `bind` — the `Ok(` wrapping the whole
`...checked(...)?.map(...)` is a bit hard to parse. A let-binding first reads
clearer:
```rust
let field = schema.field_by_name_case_insensitive_checked(column_name)?;
Ok(field.map(|f| f.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]
