Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Core: Refactor Table Metadata Tests #11947

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open

Core: Refactor Table Metadata Tests #11947

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
063b7f5
Add row-lineage fields and next-row-id
HonahX Dec 5, 2024
92c616b
remove unnecessary test
HonahX Dec 5, 2024
74bc724
add unit tests for new fields
HonahX Dec 9, 2024
baf6e0b
add v3 test metadata json
HonahX Dec 9, 2024
4786fbe
add tests for v3 row lineage parsing
HonahX Dec 9, 2024
4d5c87d
Merge branch 'main' into honahx_table_metadata_v3_parse_and_test_2
HonahX Jan 8, 2025
8f667de
Merge branch 'main' into honahx_table_metadata_v3_parse_and_test_2
HonahX Jan 10, 2025
4f50370
Fix Compilation error
HonahX Jan 10, 2025
2551587
remove fields for row lineage
HonahX Jan 10, 2025
20c72d0
First try: Add Metadata Builder for tests
HonahX Jan 11, 2025
4adb693
fix style issue
HonahX Jan 11, 2025
32ffbfd
simplify construction of table metadata in tests
HonahX Jan 11, 2025
4aa3158
Add BaseSnapshotBuilder
HonahX Jan 11, 2025
6e9b00e
switch to FieldSource in TestHelpers
HonahX Jan 15, 2025
d22a24b
Add defaults for snapshot test builder
HonahX Jan 15, 2025
1bc5075
add defaults to Table metadata test builder
HonahX Jan 15, 2025
34c480a
add constant to specify min version
HonahX Jan 15, 2025
b2c685f
Merge branch 'main' into honahx_table_metadata_v3_parse_and_test_2
HonahX Jan 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions core/src/main/java/org/apache/iceberg/TableMetadataParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -352,6 +352,7 @@ public static TableMetadata fromJson(String metadataLocation, JsonNode node) {
ImmutableList.Builder<Schema> builder = ImmutableList.builder();
for (JsonNode schemaNode : schemaArray) {
Schema current = SchemaParser.fromJson(schemaNode);
Schema.checkCompatibility(current, formatVersion);
HonahX marked this conversation as resolved.
Show resolved Hide resolved
if (current.schemaId() == currentSchemaId) {
schema = current;
}
Expand All @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

currentSchemaId = schema.schemaId();
schemas = ImmutableList.of(schema);
}
Expand Down
336 changes: 336 additions & 0 deletions core/src/test/java/org/apache/iceberg/MetadataTestUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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 {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@RussellSpitzer RussellSpitzer Jan 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a better name is UnsafeTableMetadataBuilder?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@HonahX HonahX Jan 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

private String metadataLocation;
private int formatVersion;
private String uuid;
private String location;
private long lastSequenceNumber;
private Long lastUpdatedMillis;
private int lastColumnId;
private int currentSchemaId;
private List<Schema> schemas;
private int defaultSpecId;
private List<PartitionSpec> specs;
private int lastAssignedPartitionId;
private int defaultSortOrderId;
private List<SortOrder> sortOrders;
private Map<String, String> properties;
private long currentSnapshotId;
private List<HistoryEntry> snapshotLog;
private List<TableMetadata.MetadataLogEntry> previousFiles;
private List<StatisticsFile> statisticsFiles;
private List<PartitionStatisticsFile> partitionStatisticsFiles;
private List<MetadataUpdate> changes;
private SerializableSupplier<List<Snapshot>> snapshotsSupplier;
private List<Snapshot> snapshots;
private Map<String, SnapshotRef> refs;

private TableMetadataBuilder(int formatVersion) {
this.formatVersion = formatVersion;
this.uuid = UUID.randomUUID().toString();
this.lastSequenceNumber = INITIAL_SEQUENCE_NUMBER;
this.lastUpdatedMillis = System.currentTimeMillis();
this.lastColumnId = -1;
this.currentSchemaId = -1;
this.schemas = Lists.newArrayList();
this.defaultSpecId = -1;
this.specs = Lists.newArrayList();
this.lastAssignedPartitionId = 999;
this.defaultSortOrderId = -1;
this.sortOrders = Lists.newArrayList();
this.properties = Maps.newHashMap();
this.snapshots = Lists.newArrayList();
this.currentSnapshotId = -1;
this.changes = Lists.newArrayList();
this.snapshotLog = Lists.newArrayList();
this.previousFiles = Lists.newArrayList();
this.refs = Maps.newHashMap();
this.statisticsFiles = Lists.newArrayList();
this.partitionStatisticsFiles = Lists.newArrayList();
}

public TableMetadataBuilder setMetadataLocation(String metadataFileLocation) {
this.metadataLocation = metadataFileLocation;
return this;
}

public TableMetadataBuilder setFormatVersion(int formatVersion) {
this.formatVersion = formatVersion;
return this;
}

public TableMetadataBuilder setUuid(String uuid) {
this.uuid = uuid;
return this;
}

public TableMetadataBuilder setLocation(String location) {
this.location = location;
return this;
}

public TableMetadataBuilder setLastSequenceNumber(long lastSequenceNumber) {
this.lastSequenceNumber = lastSequenceNumber;
return this;
}

public TableMetadataBuilder setLastUpdatedMillis(long lastUpdatedMillis) {
this.lastUpdatedMillis = lastUpdatedMillis;
return this;
}

public TableMetadataBuilder setLastColumnId(int lastColumnId) {
this.lastColumnId = lastColumnId;
return this;
}

public TableMetadataBuilder setCurrentSchemaId(int currentSchemaId) {
this.currentSchemaId = currentSchemaId;
return this;
}

public TableMetadataBuilder setSchemas(List<Schema> schemas) {
this.schemas = schemas;
return this;
}

public TableMetadataBuilder setDefaultSpecId(int defaultSpecId) {
this.defaultSpecId = defaultSpecId;
return this;
}

public TableMetadataBuilder setSpecs(List<PartitionSpec> specs) {
this.specs = specs;
return this;
}

public TableMetadataBuilder setLastAssignedPartitionId(int lastAssignedPartitionId) {
this.lastAssignedPartitionId = lastAssignedPartitionId;
return this;
}

public TableMetadataBuilder setDefaultSortOrderId(int defaultSortOrderId) {
this.defaultSortOrderId = defaultSortOrderId;
return this;
}

public TableMetadataBuilder setSortOrders(List<SortOrder> sortOrders) {
this.sortOrders = sortOrders;
return this;
}

public TableMetadataBuilder setProperties(Map<String, String> properties) {
this.properties = properties;
return this;
}

public TableMetadataBuilder setCurrentSnapshotId(long snapshotId) {
this.currentSnapshotId = snapshotId;
return this;
}

public TableMetadataBuilder setSnapshotsSupplier(
SerializableSupplier<List<Snapshot>> snapshotsSupplier) {
this.snapshotsSupplier = snapshotsSupplier;
return this;
}

public TableMetadataBuilder setSnapshots(List<Snapshot> snapshots) {
this.snapshots = snapshots;
return this;
}

public TableMetadataBuilder setSnapshotLog(List<HistoryEntry> snapshotLog) {
this.snapshotLog = snapshotLog;
return this;
}

public TableMetadataBuilder setMetadataHistory(
List<TableMetadata.MetadataLogEntry> metadataHistory) {
this.previousFiles = metadataHistory;
return this;
}

public TableMetadataBuilder setRefs(Map<String, SnapshotRef> refs) {
this.refs = refs;
return this;
}

public TableMetadataBuilder setChanges(List<MetadataUpdate> changes) {
this.changes = changes;
return this;
}

public TableMetadataBuilder setStatisticsFiles(List<StatisticsFile> statisticsFiles) {
this.statisticsFiles = statisticsFiles;
return this;
}

public TableMetadataBuilder setPartitionStatisticsFiles(
List<PartitionStatisticsFile> partitionStatisticsFiles) {
this.partitionStatisticsFiles = partitionStatisticsFiles;
return this;
}

public TableMetadata build() {
return new TableMetadata(
metadataLocation,
formatVersion,
uuid,
location,
lastSequenceNumber,
lastUpdatedMillis,
lastColumnId,
currentSchemaId,
ImmutableList.copyOf(schemas),
defaultSpecId,
ImmutableList.copyOf(specs),
lastAssignedPartitionId,
defaultSortOrderId,
ImmutableList.copyOf(sortOrders),
ImmutableMap.copyOf(properties),
currentSnapshotId,
ImmutableList.copyOf(snapshots),
snapshotsSupplier,
ImmutableList.copyOf(snapshotLog),
ImmutableList.copyOf(previousFiles),
ImmutableMap.copyOf(refs),
ImmutableList.copyOf(statisticsFiles),
ImmutableList.copyOf(partitionStatisticsFiles),
ImmutableList.copyOf(changes));
}
}

public static BaseSnapshotBuilder buildTestSnapshot() {
return new BaseSnapshotBuilder();
}

public static class BaseSnapshotBuilder {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this class needed?

Copy link
Contributor Author

@HonahX HonahX Jan 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@RussellSpitzer RussellSpitzer Jan 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a better name would be TestSnapshotBuilder or UnsafeSnapshotBuilder?

private long snapshotId;
private Long parentId;
private long sequenceNumber;
private long timestampMillis;
private String manifestListLocation;
private String operation;
private Map<String, String> summary;
private Integer schemaId;
private String[] v1ManifestLocations;

private BaseSnapshotBuilder() {
this.snapshotId = -1L;
this.sequenceNumber = -1L;
this.timestampMillis = System.currentTimeMillis();
this.summary = ImmutableMap.of();
}

public BaseSnapshotBuilder setSnapshotId(long snapshotId) {
this.snapshotId = snapshotId;
return this;
}

public BaseSnapshotBuilder setParentId(Long parentId) {
this.parentId = parentId;
return this;
}

public BaseSnapshotBuilder setSequenceNumber(long sequenceNumber) {
this.sequenceNumber = sequenceNumber;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return this;
}

public BaseSnapshotBuilder setTimestampMillis(long timestampMillis) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this field default?

this.timestampMillis = timestampMillis;
return this;
}

public BaseSnapshotBuilder setManifestListLocation(String manifestListLocation) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

this.manifestListLocation = manifestListLocation;
return this;
}

public BaseSnapshotBuilder setOperation(String operation) {
this.operation = operation;
return this;
}

public BaseSnapshotBuilder setSummary(Map<String, String> summary) {
this.summary = summary;
return this;
}

public BaseSnapshotBuilder setSchemaId(Integer schemaId) {
this.schemaId = schemaId;
return this;
}

public BaseSnapshotBuilder setV1ManifestLocations(String[] v1ManifestLocations) {
this.v1ManifestLocations = v1ManifestLocations;
return this;
}

public Snapshot build() {
Preconditions.checkArgument(
manifestListLocation != null || v1ManifestLocations != null,
"Cannot set both ManifestListLocation and V1ManifestLocations");
if (v1ManifestLocations != null) {
return new BaseSnapshot(
sequenceNumber,
snapshotId,
parentId,
timestampMillis,
operation,
summary,
schemaId,
v1ManifestLocations);
}
return new BaseSnapshot(
sequenceNumber,
snapshotId,
parentId,
timestampMillis,
operation,
summary,
schemaId,
manifestListLocation);
}
}
}
24 changes: 20 additions & 4 deletions core/src/test/java/org/apache/iceberg/TestDataTaskParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,10 +193,26 @@ private DataTask createDataTask() {

List<Snapshot> snapshots =
Arrays.asList(
new BaseSnapshot(
1L, 1L, null, 1234567890000L, "append", summary1, 1, "file:/tmp/manifest1.avro"),
new BaseSnapshot(
2L, 2L, 1L, 9876543210000L, "append", summary2, 1, "file:/tmp/manifest2.avro"));
MetadataTestUtils.buildTestSnapshot()
.setSequenceNumber(1L)
.setSnapshotId(1L)
.setParentId(null)
.setTimestampMillis(1234567890000L)
.setOperation("append")
.setSummary(summary1)
.setSchemaId(1)
.setManifestListLocation("file:/tmp/manifest1.avro")
.build(),
MetadataTestUtils.buildTestSnapshot()
.setSequenceNumber(2L)
.setSnapshotId(2L)
.setParentId(1L)
.setTimestampMillis(9876543210000L)
.setOperation("append")
.setSummary(summary2)
.setSchemaId(1)
.setManifestListLocation("file:/tmp/manifest2.avro")
.build());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


return StaticDataTask.of(
Files.localInput("file:/tmp/metadata2.json"),
Expand Down
Loading