rdblue commented on code in PR #11947: URL: https://github.com/apache/iceberg/pull/11947#discussion_r1917421391
########## core/src/test/java/org/apache/iceberg/MetadataTestUtils.java: ########## @@ -0,0 +1,336 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.iceberg; + +import static org.apache.iceberg.TableMetadata.INITIAL_SEQUENCE_NUMBER; + +import java.util.List; +import java.util.Map; +import java.util.UUID; +import org.apache.iceberg.relocated.com.google.common.base.Preconditions; +import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList; +import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap; +import org.apache.iceberg.relocated.com.google.common.collect.Lists; +import org.apache.iceberg.relocated.com.google.common.collect.Maps; +import org.apache.iceberg.util.SerializableSupplier; + +public class MetadataTestUtils { + + private MetadataTestUtils() {} + + public static TableMetadataBuilder buildTestTableMetadata(int formatVersion) { + return new TableMetadataBuilder(formatVersion); + } + + public static class TableMetadataBuilder { Review Comment: After looking through the tests, I'm a bit skeptical about the value of this change. I agree with making it easier to build these objects, but now it seems harder to read the tests because they are more verbose and it isn't clear how the builder methods interact. In some places, the snapshots and table metadata are built with each value explicitly, but in other places they are filled with example values and then overridden. That makes me unsure about what the tests are testing and that they are correct. For instance, if a new field is added to the parser, it would be easy to see that the test case doesn't need to change because we use example data. But if this builder isn't updated then it could easily not exercise the parser because `TableMetadata` always uses a builder that fills in default values. I liked the existing code where a new field in `TableMetadata` required a change in tests to supply the new field's value. Then it was clear that the new field had to be handled by the parser. -- 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
