stevenzwu commented on code in PR #10678:
URL: https://github.com/apache/iceberg/pull/10678#discussion_r1702369242
##########
api/src/main/java/org/apache/iceberg/types/TypeUtil.java:
##########
@@ -182,11 +181,10 @@ public static Map<Integer, String> indexQuotedNameById(
}
public static Map<String, Integer> indexByLowerCaseName(Types.StructType
struct) {
- Map<String, Integer> indexByLowerCaseName = Maps.newHashMap();
+ ImmutableMap.Builder<String, Integer> builder = ImmutableMap.builder();
indexByName(struct)
- .forEach(
- (name, integer) ->
indexByLowerCaseName.put(name.toLowerCase(Locale.ROOT), integer));
- return indexByLowerCaseName;
+ .forEach((name, integer) -> builder.put(name.toLowerCase(Locale.ROOT),
integer));
+ return builder.buildOrThrow();
Review Comment:
Instead of depending on ImmutableMap builder for the dup check, I am
wondering if we should handle the collision check here in order to throw an
exception with more informative error msg than `IllegalArgumentException:
Multiple entries with same key: a=2 and a=1`. E.g., the error msg can be sth
like `Unable to build case insensitive mapping from field name to field id
because two fields have the same lower case name...`. We can also get the
original field name since we know the two field ids and have the original
schema/struct.
also let's add Javadoc to this method to document the behavior under
collision.
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
