Core: Fix create v1 table on REST Catalog #10369

hantangwangd · 2024-05-23T09:23:18Z

This PR fix the problems of Cannot downgrade v2 table to v1 when creating v1 table on REST Catalog.

Fix issue: #8756

hantangwangd · 2024-05-28T12:17:39Z

Hi @nastra, can you please take a look at this fix when available? Thanks a lot!

nastra · 2024-06-03T13:53:23Z

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

@@ -861,6 +862,19 @@ public static Builder buildFromEmpty() {
    return new Builder();
  }

+  public static Builder buildFromMetadataUpdates(List<MetadataUpdate> metadataUpdates) {


The fix looks correct but I don't think we should expose this in TableMetadata. It should be fine to have this inside CatalogHandlers#create

Thanks for your suggestion, that seems better, fixed!

nastra · 2024-06-03T13:54:43Z

core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java

+            SCHEMA,
+            PartitionSpec.unpartitioned(),
+            ImmutableMap.of("format-version", formatVersion));
+    assertThatNoException().isThrownBy(() -> createTableTransaction1.commitTransaction());


rather than checking that no exception was thrown it would be better to just load the table and ensure it exists.

Something like this short piece should be enough:

@ParameterizedTest @ValueSource(ints = {1, 2}) public void createTableTransaction(int formatVersion) { RESTCatalogAdapter adapter = Mockito.spy(new RESTCatalogAdapter(backendCatalog)); RESTCatalog catalog = catalog(adapter); if (requiresNamespaceCreate()) { catalog.createNamespace(NS); } catalog .newCreateTableTransaction( TABLE, SCHEMA, PartitionSpec.unpartitioned(), ImmutableMap.of("format-version", String.valueOf(formatVersion))) .commitTransaction(); BaseTable table = (BaseTable) catalog.loadTable(TABLE); assertThat(table.operations().current().formatVersion()).isEqualTo(formatVersion); }

Can you please also update the other test accordingly?

In fact we should move this to CatalogTests, so that we make sure this works across different catalog implementations:

@ParameterizedTest @ValueSource(ints = {1, 2}) public void createTableTransaction(int formatVersion) { if (requiresNamespaceCreate()) { catalog().createNamespace(NS); } catalog() .newCreateTableTransaction( TABLE, SCHEMA, PartitionSpec.unpartitioned(), ImmutableMap.of("format-version", String.valueOf(formatVersion))) .commitTransaction(); BaseTable table = (BaseTable) catalog().loadTable(TABLE); assertThat(table.operations().current().formatVersion()).isEqualTo(formatVersion); }

Tests have been moved to CatalogTests, and modified as your suggestion. Please take a look when available, thanks a lot.

nastra · 2024-06-03T13:59:42Z

core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java

@@ -374,8 +374,7 @@ private static TableMetadata create(TableOperations ops, UpdateTableRequest requ
    // the only valid requirement is that the table will be created
    request.requirements().forEach(requirement -> requirement.validate(ops.current()));

-    TableMetadata.Builder builder = TableMetadata.buildFromEmpty();
-    request.updates().forEach(update -> update.applyTo(builder));
+    TableMetadata.Builder builder = TableMetadata.buildFromMetadataUpdates(request.updates());


I would just inline the fix here when iterating updates

core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java

nastra · 2024-06-04T06:35:12Z

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

@@ -991,7 +991,7 @@ public Builder assignUUID(String newUuid) {

    // it is only safe to set the format version directly while creating tables
    // in all other cases, use upgradeFormatVersion
-    private Builder setInitialFormatVersion(int newFormatVersion) {
+    public Builder setInitialFormatVersion(int newFormatVersion) {


the changes in this PR LGTM, but we need to be careful about making this method public (see also #8381 (comment)). Let me think a bit about this to see whether there's a slightly different way to approach this

one alternative would be to lazily initialize formatVersion inside TableMetadata.Builder so that calling setInitialFormatVersion wouldn't be necessary from CatalogHandlers during table creation. Let's see what others think here
/cc @aokolnychyi @rdblue @amogh-jahagirdar @RussellSpitzer

This should not be made public.

@hantangwangd I've raised a PR to your branch https://github.com/hantangwangd/iceberg/pull/1/files, but missed @nastra on the lazy formatVersion initialization. Not sure entirely what that meant, but in my approach we add a new buildFromEmpty(int formatVersion API so that the format version is only initialized once and never mutated (unless going through an actual upgrade API call)

rdblue · 2024-06-25T15:43:29Z

core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java

+        .forEach(
+            update -> {
+              if (update instanceof UpgradeFormatVersion) {
+                builder.setInitialFormatVersion(((UpgradeFormatVersion) update).formatVersion());


I don't think this is the right way to set the format version. The initial metadata needs to be created as v1, rather than dangerously setting the version to 1 after constructing it.

@rdblue Thanks for your comment. Do you mean that we should expose something like buildFromSpecifiedVersion(formatVersion) in TableMetadata for getting a builder with specified format version, and check the updates list in advance here to get the format version before creating the builder?

Or should we use a builder that lazily initialize formatVersion as @nastra described above?

I stepped through the code/debugger with the new tests, I think the right solution is to use the TableMetadata#newTableMetadata API in the create method rather than buildFromEmpty and apply the updates. That will build the initial metadata as V1 rather than trying to mutate it later after it's built, which is what I think @rdblue is saying to avoid. I'll let him confirm though.

Or an alternative is to add a buildFromEmpty(int formatVersion) API and apply the updates. Either way, agree with the idea that the format version should be defined once upfront when building a new table metadata instance for creation rather than mutate it later.

@hantangwangd I've raised a PR to your branch https://github.com/hantangwangd/iceberg/pull/1/files to show what I mean. cc @nastra @rdblue @aokolnychyi

@amogh-jahagirdar Thank you for the change, that makes sense. I absorbed it into this PR.

nastra

This looks great, thanks for fixing @hantangwangd and @amogh-jahagirdar

github-actions bot added the core label May 23, 2024

hantangwangd force-pushed the master branch from 0a60c55 to edff902 Compare May 28, 2024 09:54

nastra reviewed Jun 3, 2024

View reviewed changes

hantangwangd force-pushed the master branch from edff902 to 3de7588 Compare June 3, 2024 18:06

nastra reviewed Jun 4, 2024

View reviewed changes

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

nastra reviewed Jun 4, 2024

View reviewed changes

hantangwangd force-pushed the master branch from 3de7588 to 224c340 Compare June 4, 2024 08:51

nastra requested review from danielcweeks, aokolnychyi and amogh-jahagirdar June 17, 2024 09:49

hantangwangd force-pushed the master branch from 224c340 to 700e645 Compare June 25, 2024 05:33

nastra added this to the Iceberg 1.6.0 milestone Jun 25, 2024

rdblue reviewed Jun 25, 2024

View reviewed changes

hantangwangd and others added 2 commits July 5, 2024 02:26

Core: Fix create v1 table on REST Catalog

9ce16c2

Core: Add TableMetadata#buildFromEmpty(formatVersion) API

f236722

hantangwangd force-pushed the master branch from 97ca66d to f236722 Compare July 4, 2024 18:33

nastra approved these changes Jul 5, 2024

View reviewed changes

nastra merged commit 24d26b6 into apache:main Jul 5, 2024
49 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Core: Fix create v1 table on REST Catalog #10369

Core: Fix create v1 table on REST Catalog #10369

hantangwangd commented May 23, 2024

hantangwangd commented May 28, 2024

nastra Jun 3, 2024 •

edited

Loading

hantangwangd Jun 3, 2024

nastra Jun 3, 2024

nastra Jun 3, 2024

hantangwangd Jun 3, 2024

nastra Jun 3, 2024

hantangwangd Jun 3, 2024

nastra Jun 4, 2024

nastra Jun 4, 2024

rdblue Jun 25, 2024

amogh-jahagirdar Jul 4, 2024

rdblue Jun 25, 2024

hantangwangd Jun 25, 2024

hantangwangd Jun 25, 2024

amogh-jahagirdar Jun 25, 2024

amogh-jahagirdar Jun 25, 2024 •

edited

Loading

amogh-jahagirdar Jul 4, 2024

hantangwangd Jul 4, 2024

nastra left a comment

Core: Fix create v1 table on REST Catalog #10369

Core: Fix create v1 table on REST Catalog #10369

Conversation

hantangwangd commented May 23, 2024

hantangwangd commented May 28, 2024

nastra Jun 3, 2024 • 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

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

amogh-jahagirdar Jun 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nastra left a comment

Choose a reason for hiding this comment

nastra Jun 3, 2024 •

edited

Loading

amogh-jahagirdar Jun 25, 2024 •

edited

Loading