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

Reply via email to