rdblue commented on code in PR #10253: URL: https://github.com/apache/iceberg/pull/10253#discussion_r1876379535
########## core/src/main/java/org/apache/iceberg/view/ViewUtil.java: ########## @@ -18,12 +18,34 @@ */ package org.apache.iceberg.view; +import java.util.List; +import java.util.concurrent.atomic.AtomicInteger; +import org.apache.iceberg.Schema; import org.apache.iceberg.catalog.TableIdentifier; +import org.apache.iceberg.types.TypeUtil; public class ViewUtil { private ViewUtil() {} public static String fullViewName(String catalog, TableIdentifier ident) { return catalog + "." + ident; } + + public static Schema assignFreshIds(ViewMetadata base, Schema newSchema) { Review Comment: I think that the current arguments to this method pass too much state. Utility methods should require a minimal amount of information so it is clear what they depend on when called. Rather than passing the entire `ViewMetadata`, this method should accept `newSchema`, the base schema, and the highest field ID. I'm also not sure that it makes sense to have a util method that just calls another util method, `TypeUtil.assignFreshIds`. I think once you apply the principle of passing minimal state, you basically end up with a wrapper around the `TypeUtil` method that creates an atomic integer. -- 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