Core: Refactor Table Metadata Tests #11947

HonahX · 2025-01-10T18:38:25Z

~~Add new unit tests for V3 metadata~~
~~Refactor TableMetadata tests by parametrizing table format versions in all tests~~

[Updated] The above 2 will be included in a separate PR: #12025

Add MetadataTestUtils.TableMetadataBuilder and MetadataTestUtils.BaseSnapshotBuilder to simply test metadata and snapshot build. This will reduce the amount of test code changes when adding new fields to metadata/snapshot.

Not included:

Support for unknown V3 schema data type (Tracked by: Support UnknownType for V3 Schema #11732 )
Support for source-ids for V3 partition spec (Tracked by: Handling of Source Id => Source Ids #10762)
Support for row lineage

Parameterize TestTableMetadata

# Conflicts: # core/src/test/java/org/apache/iceberg/TestTableMetadata.java

core/src/main/java/org/apache/iceberg/TableMetadata.java

HonahX · 2025-01-10T20:31:36Z

cc @RussellSpitzer @flyrain
May I ask for your help to review this?

core/src/main/java/org/apache/iceberg/TableMetadataParser.java

HonahX · 2025-01-11T06:17:59Z

core/src/test/java/org/apache/iceberg/MetadataTestUtils.java

+    return new TableMetadataBuilder(formatVersion);
+  }
+
+  public static class TableMetadataBuilder {


I added test-only builders for TableMetadata and BaseSnapshot. This shall reduce the amount of code changes in tests when adding new fields.

cc: @RussellSpitzer

why do we need this class and why can't we just use the normal table metadata builder?

I noted this in the other comment, but our normal builder has safety checks which prohibit us from building invalid or arbitrary Table Metadata. This makes it impossible to use in testing situations where we may want to do something like force the snapshot id or put invalid variables into the TableMetadata.

Maybe a better name is UnsafeTableMetadataBuilder?

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.

That's actually why I prefer the builder, I believe the tests are much more readable without specifying all of the parameters each time and we would still force all tests to have to deal with new parameters if they all use the builder that defaults to an example fully-non-null metadata.

Previously, the worst case scenario of this approach (having to set every single property) was required for every single test. First, several local variables are created and set to arbitrary default values, seldomly these values are relevant to the test, then we call a constructor which has many arguments mostly null or empty.

For me this made every test a bit of a hunt and check, which args are being set explicitly because of this test and which just need to be non-null so that the test doesn't break? Previously, I had no idea what some tests were testing or if they were correct without IDE hints on which arg was which. For example, the test for UUID being null sets 3/ 30~ args to null and sets specific values to 13 of the args. Of all of these values only 1 of those nulls actually matters to the test and everything else is essentially noise.

If we do want to stick with using the full constructor everywhere I think that's fine but if our rational is that we don't want to force these tests to be able to ignore constructor changes then I think we need to never have secondary constructors for TableMetadata. I know adding more constructors is a a common review comment when we add new args and I think we should just articulate that we won't do that for these key constructors in Metadata and BaseSnapshot.

I think we can approach this from two different perspectives, depending on the purpose of the test.

Type 1: Comprehensive Tests Covering All or Most Fields

These tests ensure that TableMetadata as a whole is correctly processed, typically verifying round-trip serialization, deserialization, and backward compatibility. Examples include:

testJsonConversion – Ensures that all fields are correctly handled in serialization and deserialization.

testBackwardCompatibility – Verifies that metadata remains compatible across versions.

For these tests, I agree that continuing to use the constructor directly makes sense. Since these tests require explicitly setting all fields, any new field will naturally require an update, ensuring that the test remains valid and that new fields are properly incorporated.

Type 2: Targeted Tests for Specific Features or Edge Cases

These tests focus on specific behaviors or individual fields rather than the full metadata structure. For example:

testInvalidMainBranch – Only checks whether currentSnapshotId matches the main branch.

testJsonWithPreviousMetadataLog – Specifically validates how the metadataLog field is handled.

For these tests, I think the builder approach provides a readability advantage. It allows us to set only the relevant fields, making it clear what the test is actually focusing on while avoiding unnecessary details. This helps maintain clarity and ensures that unrelated fields don’t introduce noise into the test.

In conclusion, introducing the builder can significantly simplify unit tests with a more focused scope, while the constructor remains useful for comprehensive tests. One key benefit of this mixed approach is that comprehensive tests are relatively few, meaning we can still reduce the overall effort required when adding new fields.

There are also opportunities to further refine this PR by eliminating unnecessary setters. If we decide to move forward with this approach, I can update the implementation accordingly. Looking forward to your thoughts!

core/src/test/resources/TableMetadataV2StatisticsFiles.json

nastra · 2025-01-14T09:50:57Z

core/src/test/java/org/apache/iceberg/MetadataTestUtils.java

+    return new BaseSnapshotBuilder();
+  }
+
+  public static class BaseSnapshotBuilder {


why is this class needed?

This class is added to reduce the amount of code changes required in tests when we add new fields to BaseSnapshot (example)

With this class, we only have to update the builder instead of updating all the test that calls BaseSnapshot constructor.

I'm strongly in favor of adding this. Our real builder in TableMetadata has too many safety valves which makes it very difficult to build TableMetadata objects which violate those rules (which we need to do to test certain failure modes)

Wrong thread, moved up. Here I just think we'll make it much easier to build tests without stating all the args over and over.

I think a better name would be TestSnapshotBuilder or UnsafeSnapshotBuilder?

core/src/main/java/org/apache/iceberg/TableMetadataParser.java

core/src/test/java/org/apache/iceberg/TestMetadataUpdateParser.java

core/src/test/java/org/apache/iceberg/TestSnapshotJson.java

core/src/test/java/org/apache/iceberg/TestTableMetadata.java

RussellSpitzer · 2025-01-14T19:48:38Z

core/src/test/java/org/apache/iceberg/TestTableMetadata.java

  @SuppressWarnings("MethodLength")
-  public void testJsonConversion() throws Exception {
+  public void testJsonConversion(int formatVersion) throws Exception {


I'm not sure it's clear what this is testing anymore and why it is restricted to 2 or higher? I think we may need to spend a little more time on this one to be clear about what we are testing?

Yeah I should've paid more attention to this. It is actually just the last sequence number need to be set to 0 for v1 metadata. I've updated the test to remove the restriction.

RussellSpitzer · 2025-01-14T20:39:39Z

core/src/test/java/org/apache/iceberg/TestTableMetadata.java

-            statisticsFiles,
-            partitionStatisticsFiles,
-            ImmutableList.of());
+        MetadataTestUtils.buildTestTableMetadata(formatVersion)


Another one where I think if we have more interesting defaults for TestTableMetadata, we can reduce our constructors here

I added a buildTestTableMetadataWithExampleValues similar to the one for snapshot. Now we only have to call necessary setters only in each test.

RussellSpitzer · 2025-01-14T20:54:20Z

core/src/test/java/org/apache/iceberg/TestTableMetadata.java

+  @ParameterizedTest
+  @MethodSource("formatVersionsProvider")
+  public void testInvalidMainBranch(int formatVersion) throws IOException {
+    assumeThat(formatVersion).isGreaterThanOrEqualTo(2);


I feel like this should have a Constant in TableMetadata. I think we should add in a BRANCHING_MIN_SUPPORT_VERSION or whatnot

I added

static final int BRANCHING_MIN_SUPPORT_VERSION = 2; static final int UUID_REQUIRED_MIN_VERSION = 2;

Do you think we need constants for other cases like

iceberg/core/src/test/java/org/apache/iceberg/TestTableMetadata.java

Lines 719 to 748 in 34c480a

@ParameterizedTest

@FieldSource("org.apache.iceberg.TestHelpers#ALL_VERSIONS")

public void testParserV2PartitionSpecsValidation(int formatVersion) throws Exception {

assumeThat(formatVersion).isGreaterThanOrEqualTo(2);

String unsupportedVersion =

readTableMetadataInputFile(

String.format("TableMetadataV%sMissingPartitionSpecs.json", formatVersion));

assertThatThrownBy(() -> TableMetadataParser.fromJson(unsupportedVersion))

.isInstanceOf(IllegalArgumentException.class)

.hasMessage(String.format("partition-specs must exist in format v%s", formatVersion));

}

@ParameterizedTest

@FieldSource("org.apache.iceberg.TestHelpers#ALL_VERSIONS")

public void testParserLastAssignedFieldIdValidation(int formatVersion) throws Exception {

assumeThat(formatVersion).isGreaterThanOrEqualTo(2);

String unsupportedVersion =

readTableMetadataInputFile(

String.format("TableMetadataV%sMissingLastPartitionId.json", formatVersion));

assertThatThrownBy(() -> TableMetadataParser.fromJson(unsupportedVersion))

.isInstanceOf(IllegalArgumentException.class)

.hasMessage(String.format("last-partition-id must exist in format v%s", formatVersion));

}

@ParameterizedTest

@FieldSource("org.apache.iceberg.TestHelpers#ALL_VERSIONS")

public void testParserSortOrderValidation(int formatVersion) throws Exception {

assumeThat(formatVersion).isGreaterThanOrEqualTo(2);

My concern is that if we do so we will end up with lots of constant for all the new features starting from v2

RussellSpitzer

Looks mostly good to me. I really appreciate the utility methods. I have a few nits which I hope will clean things up.

RussellSpitzer · 2025-01-14T20:55:34Z

core/src/test/java/org/apache/iceberg/TestTableMetadata.java

+  @ParameterizedTest
+  @MethodSource("formatVersionsProvider")
+  public void testMainWithoutCurrent(int formatVersion) throws IOException {
+    assumeThat(formatVersion).isGreaterThanOrEqualTo(2);


Branch support constant here again

RussellSpitzer · 2025-01-14T20:56:44Z

core/src/test/java/org/apache/iceberg/TestTableMetadata.java

+  @ParameterizedTest
+  @MethodSource("formatVersionsProvider")
+  public void testV2UUIDValidation(int formatVersion) {
+    assumeThat(formatVersion).isGreaterThanOrEqualTo(2);


I think maybe this needs a constant too?

rdblue · 2025-01-15T22:05:27Z

core/src/main/java/org/apache/iceberg/TableMetadataParser.java

@@ -372,6 +373,7 @@ public static TableMetadata fromJson(String metadataLocation, JsonNode node) {
          formatVersion == 1, "%s must exist in format v%s", SCHEMAS, formatVersion);

      schema = SchemaParser.fromJson(JsonUtil.get(SCHEMA, node));
+      Schema.checkCompatibility(schema, formatVersion);


@HonahX, when I introduced this compatibility check, I purposely called it at write time rather than at read time. I've also pushed back on strictness in the read path like this in the REST catalog and other places. The reason for it is that we don't want to immediately reject metadata on read because that prevents the library from being used to debug or fix metadata issues.

For example, this check validates that initial default values are not used in v2 tables. But what if a table is created by another library that way? Checking compatibility here prevents loading the table at all, let alone using this library to drop the problematic column from the schema using SQL DDL or direct calls. Failing to load the table has much broader consequences, like failing existence checks because tableExists calls loadTable, failing to run information_schema queries that are unrelated, or failing to run expireSnapshots and remove old data -- which can cause compliance problems.

I think that a better time to fail is when the actual problem caused by the compatibility issue happens. For instance, if there is a default that can't be applied, then the library should fail to read from the table.

Thanks for the explanation! So in general we want to be less restrictive on read side to open the opportunities of fix things instead of rejecting all the errors.

I will revert this change.

I think if this is the case we should have some tests that show that parsing invalid metadata is a behavior is allowed by the library. Parsing some invalid Json should not throw an exception for compatibility purposes? I think we could just take a fully populated V3 Metadata and change it's format version to 1 or something. This should be readable (but not really usable)? I'm not sure what other cases we would want, but I think we'd be in a better state if we had tests for behaviors we want to keep in the code.

RussellSpitzer · 2025-01-15T22:12:09Z

core/src/test/java/org/apache/iceberg/TestSnapshotJson.java

-        new BaseSnapshot(
-            0, snapshotId, parentId, System.currentTimeMillis(), null, null, 1, manifestList);
+        MetadataTestUtils.buildTestSnapshotWithExampleValues()
+            .setOperation(null)


I don't think operation, summary or schema ID really need to be specified here right?

rdblue · 2025-01-15T22:15:36Z

core/src/test/java/org/apache/iceberg/MetadataTestUtils.java

+    return new BaseSnapshotBuilder();
+  }
+
+  public static BaseSnapshotBuilder buildTestSnapshotWithExampleValues() {


I would expect this to be a configuration method on the builder, not a factory method that returns a builder. I think it would be more clear that way. Here's what I saw below:

MetadataTestUtils.buildTestSnapshotWithExampleValues() .buildWithExampleManifestList( temp, ImmutableList.of( MetadataTestUtils.EXAMPLE_MANIFEST_PATH_1, MetadataTestUtils.EXAMPLE_MANIFEST_PATH_2));

Instead, I think it would make more sense like this:

MetadataTestUtils.unsafeMetadataBuilder() .withExampleValues() .withExampleManifestList(EXAMPLE_MANIFEST_PATH_1, EXAMPLE_MANIFEST_PATH_2) .build();

Also, is it necessary to use an unsafe builder for these cases? The justification for the builder was to be able to build invalid cases, but these are valid.

RussellSpitzer · 2025-01-15T22:16:04Z

core/src/test/java/org/apache/iceberg/TestTableMetadata.java

-            partitionStatisticsFiles,
-            ImmutableList.of());
+        MetadataTestUtils.buildTestTableMetadataWithExampleValues(formatVersion)
+            .setCurrentSchemaId(7)


we shouldn't really need any of these if they are in TestTableMetadataWithExampleValues since this is a ser/de test

rdblue · 2025-01-15T22:17:36Z

core/src/test/java/org/apache/iceberg/MetadataTestUtils.java

+    }
+
+    public BaseSnapshotBuilder setSequenceNumber(long sequenceNumber) {
+      this.sequenceNumber = sequenceNumber;


Because this is a long primitive, it cannot mimic v1 snapshots.

rdblue · 2025-01-15T22:17:50Z

core/src/test/java/org/apache/iceberg/MetadataTestUtils.java

+      return this;
+    }
+
+    public BaseSnapshotBuilder setTimestampMillis(long timestampMillis) {


Should this field default?

rdblue · 2025-01-15T22:18:45Z

core/src/test/java/org/apache/iceberg/MetadataTestUtils.java

+      return this;
+    }
+
+    public BaseSnapshotBuilder setManifestListLocation(String manifestListLocation) {


What about making this similar to setV1ManifestLocations instead and combining it with buildWithExampleManifestList? I think that would make more sense.

rdblue · 2025-01-15T22:18:52Z

core/src/test/java/org/apache/iceberg/MetadataTestUtils.java

+          manifestListLocation);
+    }
+
+    public Snapshot buildWithExampleManifestList(Path temp, List<String> manifestFiles)


I'd expect this to be a normal method on the builder. Why put it here instead?

rdblue · 2025-01-15T22:19:34Z

core/src/test/java/org/apache/iceberg/TestDataTaskParser.java

+                .setSummary(summary2)
+                .setSchemaId(1)
+                .setManifestListLocation("file:/tmp/manifest2.avro")
+                .build());


It doesn't seem to me that this strictly needs to be changed, but I'm okay with it for readability.

rdblue · 2025-01-15T22:22:46Z

core/src/test/java/org/apache/iceberg/TestSnapshotJson.java

+        MetadataTestUtils.buildTestSnapshotWithExampleValues()
+            .setOperation(null)
+            .setSummary(null)
+            .setSchemaId(null)


Should this set operation and summary? Seems like only schema ID is needed.

rdblue · 2025-01-15T22:23:39Z

core/src/test/java/org/apache/iceberg/TestSnapshotJson.java

    Snapshot expected =
-        new BaseSnapshot(
-            0, snapshotId, parentId, System.currentTimeMillis(), null, null, null, manifestList);
+        MetadataTestUtils.buildTestSnapshotWithExampleValues()


Since this is used to fill base data and then the methods are used to override, should example values be the default behavior? That would make more sense to me.

HonahX · 2025-01-21T23:57:24Z

Hi everyone, thanks for all the review and suggestions! Since there is an ongoing discussion on whether to add test-only builders for Snapshot and TableMetadata, I created a new PR, #12025, for the following changes:

Refactor TableMetadata tests by parametrizing table format versions in all tests
Add example metadata files for V3 table
Add constants for several features/fields' min format version

I will update the title and description of this PR so that we can focus on the test-only builders in this one.

HonahX added 6 commits December 5, 2024 15:09

Add row-lineage fields and next-row-id

063b7f5

Parameterize TestTableMetadata

remove unnecessary test

92c616b

add unit tests for new fields

74bc724

add v3 test metadata json

baf6e0b

add tests for v3 row lineage parsing

4786fbe

Merge branch 'main' into honahx_table_metadata_v3_parse_and_test_2

4d5c87d

# Conflicts: # core/src/test/java/org/apache/iceberg/TestTableMetadata.java

github-actions bot added the core label Jan 10, 2025

HonahX added 2 commits January 10, 2025 10:43

Merge branch 'main' into honahx_table_metadata_v3_parse_and_test_2

8f667de

Fix Compilation error

4f50370

HonahX marked this pull request as ready for review January 10, 2025 19:54

HonahX commented Jan 10, 2025

View reviewed changes

core/src/main/java/org/apache/iceberg/TableMetadata.java Outdated Show resolved Hide resolved

flyrain reviewed Jan 10, 2025

View reviewed changes

core/src/main/java/org/apache/iceberg/TableMetadataParser.java Outdated Show resolved Hide resolved

HonahX added 5 commits January 10, 2025 15:00

remove fields for row lineage

2551587

First try: Add Metadata Builder for tests

20c72d0

fix style issue

4adb693

simplify construction of table metadata in tests

32ffbfd

Add BaseSnapshotBuilder

4aa3158

HonahX commented Jan 11, 2025

View reviewed changes

Fokko reviewed Jan 13, 2025

View reviewed changes

core/src/test/resources/TableMetadataV2StatisticsFiles.json Outdated Show resolved Hide resolved

nastra reviewed Jan 14, 2025

View reviewed changes

HonahX mentioned this pull request Jan 14, 2025

Core, API, Spec: Metadata Row Lineage #11948

Open

RussellSpitzer reviewed Jan 14, 2025

View reviewed changes

core/src/main/java/org/apache/iceberg/TableMetadataParser.java Show resolved Hide resolved

RussellSpitzer reviewed Jan 14, 2025

View reviewed changes

core/src/test/java/org/apache/iceberg/TestMetadataUpdateParser.java Outdated Show resolved Hide resolved

RussellSpitzer reviewed Jan 14, 2025

View reviewed changes

core/src/test/java/org/apache/iceberg/TestSnapshotJson.java Outdated Show resolved Hide resolved

RussellSpitzer reviewed Jan 14, 2025

View reviewed changes

core/src/test/java/org/apache/iceberg/TestSnapshotJson.java Outdated Show resolved Hide resolved

RussellSpitzer reviewed Jan 14, 2025

View reviewed changes

core/src/test/java/org/apache/iceberg/TestTableMetadata.java Outdated Show resolved Hide resolved

RussellSpitzer reviewed Jan 14, 2025

View reviewed changes

HonahX added 5 commits January 14, 2025 20:10

switch to FieldSource in TestHelpers

6e9b00e

Add defaults for snapshot test builder

d22a24b

add defaults to Table metadata test builder

1bc5075

add constant to specify min version

34c480a

Merge branch 'main' into honahx_table_metadata_v3_parse_and_test_2

b2c685f

rdblue reviewed Jan 15, 2025

View reviewed changes

RussellSpitzer reviewed Jan 15, 2025

View reviewed changes

rdblue reviewed Jan 15, 2025

View reviewed changes

RussellSpitzer reviewed Jan 15, 2025

View reviewed changes

rdblue reviewed Jan 15, 2025

View reviewed changes

HonahX mentioned this pull request Jan 21, 2025

Core, Test: Parsing and Writing Tests for V3 Metadata #12025

Open

HonahX changed the title ~~Core: Parsing and Writing Tests for V3 Metadata~~ Core: Refactor Table Metadata Tests Jan 21, 2025

	@ParameterizedTest
	@FieldSource("org.apache.iceberg.TestHelpers#ALL_VERSIONS")
	public void testParserV2PartitionSpecsValidation(int formatVersion) throws Exception {
	assumeThat(formatVersion).isGreaterThanOrEqualTo(2);

	String unsupportedVersion =
	readTableMetadataInputFile(
	String.format("TableMetadataV%sMissingPartitionSpecs.json", formatVersion));
	assertThatThrownBy(() -> TableMetadataParser.fromJson(unsupportedVersion))
	.isInstanceOf(IllegalArgumentException.class)
	.hasMessage(String.format("partition-specs must exist in format v%s", formatVersion));
	}

	@ParameterizedTest
	@FieldSource("org.apache.iceberg.TestHelpers#ALL_VERSIONS")
	public void testParserLastAssignedFieldIdValidation(int formatVersion) throws Exception {
	assumeThat(formatVersion).isGreaterThanOrEqualTo(2);

	String unsupportedVersion =
	readTableMetadataInputFile(
	String.format("TableMetadataV%sMissingLastPartitionId.json", formatVersion));
	assertThatThrownBy(() -> TableMetadataParser.fromJson(unsupportedVersion))
	.isInstanceOf(IllegalArgumentException.class)
	.hasMessage(String.format("last-partition-id must exist in format v%s", formatVersion));
	}

	@ParameterizedTest
	@FieldSource("org.apache.iceberg.TestHelpers#ALL_VERSIONS")
	public void testParserSortOrderValidation(int formatVersion) throws Exception {
	assumeThat(formatVersion).isGreaterThanOrEqualTo(2);

Core: Refactor Table Metadata Tests #11947

Are you sure you want to change the base?

Core: Refactor Table Metadata Tests #11947

Conversation

HonahX commented Jan 10, 2025 • edited Loading

HonahX commented Jan 10, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RussellSpitzer Jan 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HonahX Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

Type 1: Comprehensive Tests Covering All or Most Fields

Type 2: Targeted Tests for Specific Features or Edge Cases

Choose a reason for hiding this comment

HonahX Jan 14, 2025 • edited Loading

Choose a reason for hiding this comment

RussellSpitzer Jan 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RussellSpitzer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HonahX commented Jan 21, 2025

HonahX commented Jan 10, 2025 •

edited

Loading

RussellSpitzer Jan 14, 2025 •

edited

Loading

HonahX Jan 16, 2025 •

edited

Loading

HonahX Jan 14, 2025 •

edited

Loading

RussellSpitzer Jan 14, 2025 •

edited

Loading