Skip to content

REST: Set table format version for create table transactions #5084

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

Merged
rdblue merged 7 commits into from
Jun 21, 2022
Merged

REST: Set table format version for create table transactions #5084

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8f836b0
Shutdown refresh token thread during REST catalog client close
bryanck Jun 4, 2022
12b6349
Merge branch 'apache:master' into master
bryanck Jun 7, 2022
4670322
Improved shutdown of token refresh executor during REST catalog close
bryanck Jun 7, 2022
e9a39da
Merge remote-tracking branch 'upstream/master'
bryanck Jun 20, 2022
68b9490
REST: Set table format version for create table transactions
bryanck Jun 20, 2022
5aaf1f3
Merge remote-tracking branch 'upstream/master'
bryanck Jun 20, 2022
d029cc0
add test for creating v2 table via transaction
bryanck Jun 20, 2022
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/rest/RESTSessionCatalog.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -556,6 +556,8 @@ private LoadTableResponse stageCreate() {
private static List<MetadataUpdate> createChanges(TableMetadata meta) {
ImmutableList.Builder<MetadataUpdate> changes = ImmutableList.builder();

changes.add(new MetadataUpdate.UpgradeFormatVersion(meta.formatVersion()));
Copy link
Contributor

@rdblue rdblue Jun 20, 2022

Choose a reason for hiding this comment

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

I think this should be set only if the format version is not the default?

Copy link
Contributor Author

@bryanck bryanck Jun 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I think the server-side default and the client-side default may not match? The issue I originally encountered was the Trino plugin had v2 as the default and set the initial sequence number to 1, but the server didn't receive a format version so treated it as v1, and failed because the sequence number was > 0. If the client and server are on different Iceberg versions, there could potentially be a mismatch in the default.

Copy link
Contributor

@rdblue rdblue Jun 20, 2022

Choose a reason for hiding this comment

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

Okay, I see what you mean.

Copy link
Contributor

@kbendick kbendick Jun 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

+1.

I could have sworn I left an almost identical comment when I reviewed this before (the single comment I was referring to). I can possibly help with adding a unit test for this sequence in CatalogTests if we’d like. Feel free to reach out on Slack too.

I too see what you mean.


Schema schema = meta.schema();
changes.add(new MetadataUpdate.AddSchema(schema, schema.highestFieldId()));
changes.add(new MetadataUpdate.SetCurrentSchema(-1));
Expand Down
47 changes: 47 additions & 0 deletions core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableSet;
import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
import org.apache.iceberg.relocated.com.google.common.collect.Lists;
import org.apache.iceberg.relocated.com.google.common.collect.Maps;
import org.apache.iceberg.relocated.com.google.common.collect.Sets;
import org.apache.iceberg.relocated.com.google.common.collect.Streams;
import org.apache.iceberg.types.Types;
Expand Down Expand Up @@ -1215,6 +1216,52 @@ public void testCompleteCreateTransaction() {
assertPreviousMetadataFileCount(table, 0);
}

@Test
public void testCompleteCreateTransactionV2() {
C catalog = catalog();

Map<String, String> properties =
ImmutableMap.of("user", "someone", "created-at", "2022-02-25T00:38:19", "format-version", "2");

Transaction create = catalog.buildTable(TABLE, SCHEMA)
.withLocation("file:/tmp/ns/table")
.withPartitionSpec(SPEC)
.withSortOrder(WRITE_ORDER)
.withProperties(properties)
.createTransaction();

Assert.assertFalse("Table should not exist after createTransaction", catalog.tableExists(TABLE));

create.newFastAppend()
.appendFile(FILE_A)
.commit();

Assert.assertFalse("Table should not exist after append commit", catalog.tableExists(TABLE));

create.commitTransaction();

Assert.assertTrue("Table should exist after append commit", catalog.tableExists(TABLE));
Table table = catalog.loadTable(TABLE);

Map<String, String> expectedProps = Maps.newHashMap(properties);
expectedProps.remove("format-version");

Assert.assertEquals("Table schema should match the new schema",
TABLE_SCHEMA.asStruct(), table.schema().asStruct());
Assert.assertEquals("Table should have create partition spec", TABLE_SPEC.fields(), table.spec().fields());
Assert.assertEquals("Table should have create sort order", TABLE_WRITE_ORDER, table.sortOrder());
Assert.assertEquals("Table properties should be a superset of the requested properties",
expectedProps.entrySet(),
Sets.intersection(properties.entrySet(), table.properties().entrySet()));
Assert.assertEquals(
"Sequence number should start at 1 for v2 format", 1, table.currentSnapshot().sequenceNumber());
if (!overridesRequestedLocation()) {
Assert.assertEquals("Table location should match requested", "file:/tmp/ns/table", table.location());
}
assertFiles(table, FILE_A);
assertPreviousMetadataFileCount(table, 0);
}

@Test
public void testConcurrentCreateTransaction() {
C catalog = catalog();
Expand Down