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: [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]
