[manuzhang]

Which issue does this PR close?

• None.

What changes are included in this PR?

Case-insensitive schema lookup now records lower-cased name collisions instead of silently selecting one field. Scan projection and expression binding propagate a data-invalid error when a requested name is ambiguous, while case-sensitive lookup is unchanged.

This change was extracted from #2773 so the Unknown-type work can be reviewed independently.

Are these changes tested?

• Added a regression covering ambiguous case-insensitive scan projection and expression binding.
• cargo test -p iceberg --lib test_case_insensitive_scan_rejects_ambiguous_column_name
• cargo fmt --all -- --check
• cargo clippy -p iceberg --all-targets --all-features -- -D warnings

AI Disclosure

This PR was prepared with assistance from Codex.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[manuzhang]

@CTTY @blackmwk please help review when you find time.

[Stefan-Dienst]

Hi @manuzhang,

I am not that familiar with the codebase, but your PR was a good read.

Generally I think that erroring in the case of ambiguity is a great change. I wondered if someone may unknowingly built upon this behavior, but as this ambiguity should lead to inconsistent field selection it's unlikely.

I just had concerns over test coverage. Maybe you could also cover that:

• One can still select case insensitive unambiguous field names, even if a schema has case insensitive ambiguous field names.
• One can still build a schema with case insensitive ambiguous field names.
• General coverage for field_by_name_case_insensitive_checked

[laskoviymishka]

The direction is right — rejecting genuinely ambiguous case-insensitive names is the correct call, and the internal callers (bind, resolve_field_id) are converted cleanly. The tests Stefan asked for look like they've landed too.

What I'd want to settle before merge is the public surface. field_by_name_case_insensitive is a stable pub fn, and this quietly changes its contract: an ambiguous name now returns None, indistinguishable from "field not found" — where before it always handed back some field. The variant that can actually surface the ambiguity, _checked, is pub(crate), so external callers have no way to reach the new behavior. I'd promote _checked to pub and point the old doc comment at it, or at minimum document that None now means "missing or ambiguous."

A couple of smaller things while we're in here:

• _checked has no doc comment, and it's the right spot to note that our per-name check is intentionally a Rust-only refinement — Java is all-or-nothing (any collision poisons every case-insensitive lookup) and PyIceberg silently keeps the last collider, so cross-engine users can see three different outcomes for the same table.
• The sentinel drops the colliding IDs at build time, so the error can only name the lookup key, not what it collided with. If we want a Java-style "id and ID collide" message we'd need the map to hold the colliders — fine either way, but worth deciding now.

None of it is a big lift — sort out the public-surface piece and I think this is good to go.

[Stefan-Dienst]

Thank you for changing the tests @manuzhang . Looks good 👍

[laskoviymishka]

The public-surface piece is sorted — _checked is pub now, the old field_by_name_case_insensitive doc points at it and spells out that None means missing-or-ambiguous, and the cross-engine note (Java all-or-nothing, PyIceberg keeps one collider) is exactly the caveat I was after. That was the thing blocking me last round, so from my side this is good to go.

Left some small stuff inline — the invalid_data! macro on the new arm, a readability tidy on the old fn. None of it blocks.

Overall LGTM, will left for some time to soak before merge. Nice work on this.

[laskoviymishka]

🌬️ ⛵