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 all 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/TableMetadata.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,8 @@ public class TableMetadata implements Serializable {
static final int INITIAL_SPEC_ID = 0;
static final int INITIAL_SORT_ORDER_ID = 1;
static final int INITIAL_SCHEMA_ID = 0;
static final int BRANCHING_MIN_SUPPORT_VERSION = 2;
static final int UUID_REQUIRED_MIN_VERSION = 2;

private static final long ONE_MINUTE = TimeUnit.MINUTES.toMillis(1);

Expand Down
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
Loading