Fix incorrect metadata for Iceberg table after rollback #13704

ebyhr · 2022-08-17T08:35:53Z

Description

Documentation

(x) No documentation is needed.

Release notes

(x) Release notes entries required with the following suggested text:

# Iceberg
* Fix incorrect table definition after rollbacked. ({issue}`13699`)

findepi · 2022-08-22T12:59:35Z

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java

+    private static Schema getCurrentSnapshotSchema(Table icebergTable)
+    {
+        Optional<Long> tableSnapshotId = Optional.ofNullable(icebergTable.currentSnapshot()).map(Snapshot::snapshotId);
+        if (tableSnapshotId.isPresent() && !findLatestSnapshotId(icebergTable.snapshots()).equals(tableSnapshotId)) {


why !findLatestSnapshotId check is necessary?
please document

is it test-covered?

For instance, when changed the table definition (e.g. ADD COLUMN), tableSnapshotId still points to the old definition before adding the column. The condition is for using the latest schema in such cases.

Added a code comment and it's covered in ALTER TABLE → INSERT INTO in testRollbackSnapshotWithSchemaEvolution.

I get it now. However, consider the case

CREATE TABLE t (a varchar); INSERT INTO t VALUES 'one column' ; -- snapshot "s" -- returns `a` column SELECT * FROM t FOR VERSION AS OF s; ALTER TABLE t ADD COLUMN (b varchar); -- this should return a, b columns: SELECT * FROM t; -- and this should continue to return only the `a` column SELECT * FROM t FOR VERSION AS OF s;

Can you verify this is the case?

I think #14076 covered the case. Let me know if my understanding is wrong. Confirmed it works as

CREATE TABLE t (a varchar); INSERT INTO t VALUES 'one column' ; TABLE "t$snapshots"; committed_at | snapshot_id | parent_id | operation | > ------------------------------------+---------------------+---------------------+-----------+------------------------------------------------> 2022-09-13 15:24:59.540 Asia/Tokyo | 5659256701260247696 | NULL | append | /var/folders/8s/dkvf18z55lj_9yxhy1n54sph0000gn/> 2022-09-13 15:24:59.744 Asia/Tokyo | 261758948915485069 | 5659256701260247696 | append | /var/folders/8s/dkvf18z55lj_9yxhy1n54sph0000gn/> -- returns `a` column SELECT * FROM t FOR VERSION AS OF 261758948915485069; a ------------ one column ALTER TABLE t ADD COLUMN b varchar; ADD COLUMN -- this should return a, b columns: SELECT * FROM t; a | b ------------+------ one column | NULL -- and this should continue to return only the `a` column SELECT * FROM t FOR VERSION AS OF 261758948915485069; a ------------ one column

findepi · 2022-09-13T10:24:55Z

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java

+    private static Optional<Long> findLatestSnapshotId(Iterable<Snapshot> snapshots)
+    {
+        try {
+            return Optional.of(Iterables.getLast(snapshots).snapshotId());


Is icebergTable.snapshots() guaranteed to be ordered, or we need to sort this first?

(see #10258 (comment) )

findepi · 2022-09-13T10:25:41Z

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java

+            return Optional.of(Iterables.getLast(snapshots).snapshotId());
+        }
+        catch (NoSuchElementException e) {
+            return Optional.empty();


you can use Iterables.isEmpty() to avoid exception-drive code flow

(probably obsolete, if you also need to sort)

findepi · 2022-09-13T10:28:10Z

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java

@@ -604,7 +605,7 @@ public Iterator<TableColumnsMetadata> streamTableColumns(ConnectorSession sessio
                        }

                        Table icebergTable = catalog.loadTable(session, tableName);
-                        List<ColumnMetadata> columns = getColumnMetadatas(icebergTable.schema());
+                        List<ColumnMetadata> columns = getColumnMetadatas(getCurrentSnapshotSchema(icebergTable));


Why it's more correct than icebergTable.schema()?

With correct code, it looks like the information_schema.columns doesn't "see" newly added columns until a new data snapshot is committed.

findepi · 2022-09-13T10:31:17Z

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java

@@ -348,7 +349,7 @@ public IcebergTableHandle getTableHandle(
        }
        else {
            tableSnapshotId = Optional.ofNullable(table.currentSnapshot()).map(Snapshot::snapshotId);
-            tableSchema = table.schema();
+            tableSchema = getCurrentSnapshotSchema(table);


In https://github.com/trinodb/trino/pull/13704/files#r969206216 you're showing that this works, but i don't understand yet why it does so.

What's the situation when table.schema() is different from getCurrentSnapshotSchema(table)?

In that situation, how do they differ?

The situation is after rollback. In the last SELECT statement in testRollbackSnapshotWithSchemaEvolution,
table.schema() returns

table { 1: col0: optional int 2: col1: optional int }

getCurrentSnapshotSchema(table) returns

table { 1: col0: optional int }

findepi · 2022-10-03T09:17:27Z

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java

-        catch (NoSuchElementException e) {
-            return Optional.empty();
-        }
+        return StreamSupport.stream(snapshots.spliterator(), false)


use Guava Streams.stream

Suggested change

return StreamSupport.stream(snapshots.spliterator(), false)

return stream(snapshots)

findepi · 2022-10-03T10:31:05Z

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java

@@ -615,6 +617,24 @@ public Iterator<TableColumnsMetadata> streamTableColumns(ConnectorSession sessio
                .iterator();
    }

+    private static Schema getCurrentSnapshotSchema(Table icebergTable)


Let's add javadoc explaining how getCurrentSnapshotSchema(Table) is different from Table#schema() and when this should be used.

findepi · 2022-10-03T10:36:44Z

plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java

+                .skippingTypesCheck()
+                .matches("VALUES 'col0'");
+        assertQuery("SELECT * FROM test_rollback_with_schema_evolution", "VALUES 1");
+


add

// A DML should not change the table schema (which could be possible if the rolled back schema gets revived when committing th DML) assertUpdate("INSERT INTO test_rollback_with_schema_evolution VALUES (13)"); assertThat(query("DESCRIBE test_rollback_with_schema_evolution")) .projected(0) .skippingTypesCheck() .matches("VALUES 'col0'"); assertQuery("SELECT * FROM test_rollback_with_schema_evolution", "VALUES 1, 13");

This should pass, but actually the INSERT is failing.

It seems to me now that we cannot fix #13699 without Iceberg side being fixed (apache/iceberg#5591).

ebyhr · 2022-11-08T03:04:51Z

Let me close this PR because it requires changes in Iceberg side #13704 (comment). I will create a new PR once it's supported.

cla-bot bot added the cla-signed label Aug 17, 2022

homar approved these changes Aug 17, 2022

View reviewed changes

ebyhr requested review from electrum and findepi August 17, 2022 23:37

ebyhr mentioned this pull request Aug 17, 2022

Iceberg rollback_to_snapshot procedure doesn't rollback the table definition #13699

Open

findepi changed the title ~~Fix incorrect metadata for rollbacked Iceberg table~~ Fix incorrect metadata for Iceberg table after rollback Aug 22, 2022

findepi approved these changes Aug 22, 2022

View reviewed changes

ebyhr force-pushed the ebi/iceberg-rollbacked-metadata branch from 7f60491 to cb4e146 Compare September 8, 2022 06:45

findepi reviewed Sep 13, 2022

View reviewed changes

ebyhr added 2 commits September 28, 2022 08:33

Fix incorrect metadata for rollbacked Iceberg table

6d6776c

fixup! Fix incorrect metadata for rollbacked Iceberg table

b14b636

ebyhr force-pushed the ebi/iceberg-rollbacked-metadata branch from cb4e146 to b14b636 Compare September 27, 2022 23:50

empty

36a2814

findepi reviewed Oct 3, 2022

View reviewed changes

ebyhr closed this Nov 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix incorrect metadata for Iceberg table after rollback #13704

Fix incorrect metadata for Iceberg table after rollback #13704

ebyhr commented Aug 17, 2022 •

edited

Loading

findepi Aug 22, 2022

ebyhr Sep 8, 2022 •

edited

Loading

findepi Sep 8, 2022

ebyhr Sep 13, 2022 •

edited

Loading

findepi Sep 13, 2022

findepi Sep 13, 2022

findepi Sep 13, 2022

findepi Sep 13, 2022

ebyhr Sep 28, 2022

findepi Oct 3, 2022

findepi Oct 3, 2022

findepi Oct 3, 2022

ebyhr commented Nov 8, 2022

	return StreamSupport.stream(snapshots.spliterator(), false)
	return stream(snapshots)

Fix incorrect metadata for Iceberg table after rollback #13704

Fix incorrect metadata for Iceberg table after rollback #13704

Conversation

ebyhr commented Aug 17, 2022 • edited Loading

Description

Documentation

Release notes

Choose a reason for hiding this comment

ebyhr Sep 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebyhr Sep 13, 2022 • 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

ebyhr commented Nov 8, 2022

ebyhr commented Aug 17, 2022 •

edited

Loading

ebyhr Sep 8, 2022 •

edited

Loading

ebyhr Sep 13, 2022 •

edited

Loading