Refactor Delta's DROP TABLE support for corrupted tables #16651

findepi · 2023-03-21T16:22:04Z

Previously, a corrupted table handle was represented with a
DeltaLakeTableHandle missing a MetadataEntry. When drop support for
corrupted tables was implemented, a connector could have only one table
handle class. This commit improves the distinction by introducing a
dedicated table handle class to represent corrupted tables. As a
necessity, this class is explicitly handled in multiple
DeltaLakeMetadata methods. This sets viable example to follow for
implementing drop support for corrupted Iceberg tables as a follow-up.

Note: testGetInsertLayoutTableNotFound test was removed, instead of
being updated, since ConnectorMetadata.getInsertLayout cannot be
reached for a corrupted table, as getting column list will fail earlier.

findepi · 2023-03-21T16:22:31Z

plugin/trino-delta-lake/src/test/java/io/trino/plugin/deltalake/TestDeltaLakeBasic.java

+        assertQuerySucceeds("ALTER TABLE " + tableName + " RENAME TO bad_person_some_new_name");
+        assertQuerySucceeds("ALTER TABLE bad_person_some_new_name RENAME TO " + tableName);
+        assertQueryFails("ALTER TABLE " + tableName + " ADD COLUMN foo int", "Metadata not found in transaction log for tpch.bad_person");
+        // TODO (https://github.com/trinodb/trino/issues/16248) ADD field


I expected that the target of #16248 is only Iceberg connector. Could you replace the link with #16662?

i linked to the PR that adds syntax.
Once we have syntax supported we should have an uncommented test line here, even before the functionality is supported in Delta

findepi · 2023-03-21T16:22:41Z

plugin/trino-delta-lake/src/test/java/io/trino/plugin/deltalake/TestDeltaLakeBasic.java

+        assertQueryFails("TRUNCATE TABLE " + tableName, "This connector does not support truncating tables");
+        assertQueryFails("COMMENT ON TABLE " + tableName + " IS NULL", "Protocol entry not found in transaction log for table tpch.bad_person");
+        assertQueryFails("COMMENT ON COLUMN " + tableName + ".foo IS NULL", "Metadata not found in transaction log for tpch.bad_person");
+        assertQueryFails("CALL system.vacuum(CURRENT_SCHEMA, '" + tableName + "', '7d')", "\\QFailure when vacuuming tpch.bad_person with retention 7d: java.lang.NullPointerException: Cannot invoke \"java.util.List.stream()\" because \"recentVersions\" is null");


Note: NullPointerException here (before the refactor)

findepi · 2023-03-21T16:25:22Z

plugin/trino-delta-lake/src/test/java/io/trino/plugin/deltalake/TestDeltaLakeBasic.java

@@ -140,21 +140,20 @@ public void testCorruptedTableLocation()
        assertQueryFails("ANALYZE " + tableName, "Metadata not found in transaction log for tpch.bad_person");
        assertQueryFails("ALTER TABLE " + tableName + " EXECUTE optimize", "Metadata not found in transaction log for tpch.bad_person");
        assertQueryFails("ALTER TABLE " + tableName + " EXECUTE vacuum", "Metadata not found in transaction log for tpch.bad_person");
-        assertQuerySucceeds("ALTER TABLE " + tableName + " RENAME TO bad_person_some_new_name");


Note: before the change we allowed rename. I think it was incidental

findepi · 2023-03-21T16:25:47Z

plugin/trino-delta-lake/src/test/java/io/trino/plugin/deltalake/TestDeltaLakeMetadata.java

-                false);
-
-        assertThatThrownBy(() -> deltaLakeMetadataFactory.create(SESSION.getIdentity())
-                .getInsertLayout(SESSION, missingTableHandle))


Note: testGetInsertLayoutTableNotFound test was removed, instead of
being updated, since ConnectorMetadata.getInsertLayout cannot be
reached for a corrupted table, as getting column list will fail earlier.

findepi · 2023-03-21T22:18:07Z

CI #16658

findepi · 2023-03-21T22:18:36Z

CI ... and some bigquery failures which i don't understand. cc @ebyhr @hashhar

ebyhr · 2023-03-21T22:23:43Z

The BigQuery CI failures are handled in #16603

ebyhr · 2023-03-22T00:34:05Z

plugin/trino-delta-lake/src/test/java/io/trino/plugin/deltalake/TestDeltaLakeBasic.java

+        assertQuerySucceeds("ALTER TABLE " + tableName + " RENAME TO bad_person_some_new_name");
+        assertQuerySucceeds("ALTER TABLE bad_person_some_new_name RENAME TO " + tableName);
+        assertQueryFails("ALTER TABLE " + tableName + " ADD COLUMN foo int", "Metadata not found in transaction log for tpch.bad_person");
+        // TODO (https://github.com/trinodb/trino/issues/16248) ADD field


I expected that the target of #16248 is only Iceberg connector. Could you replace the link with #16662?

plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeMetadata.java

This makes it consistent with other methods like that, in particular with `DeltaLakeTableHandle.forOptimize`.

findepi · 2023-03-22T16:30:35Z

Conflict with #16466, rebasing

losipiuk · 2023-03-23T10:45:20Z

plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeMetadata.java

+        if (tableHandle instanceof CorruptedDeltaLakeTableHandle corruptedTableHandle) {
+            throw corruptedTableHandle.createException();
+        }


nit: you can extract throwIfCourrupted(ConnectorTableHandle handle) method

i wanted to have throw in the calling code so that code flow analysis works, but i don't think there is much gain from that.

will change

Previously, a corrupted table handle was represented with a `DeltaLakeTableHandle` missing a `MetadataEntry`. When drop support for corrupted tables was implemented, a connector could have only one table handle class. This commit improves the distinction by introducing a dedicated table handle class to represent corrupted tables. As a necessity, this class is explicitly handled in multiple `DeltaLakeMetadata` methods. This sets viable example to follow for implementing drop support for corrupted Iceberg tables as a follow-up. Note: `testGetInsertLayoutTableNotFound` test was removed, instead of being updated, since `ConnectorMetadata.getInsertLayout` cannot be reached for a corrupted table, as getting column list will fail earlier.

findepi · 2023-03-23T12:17:38Z

CI #13995 and #16694 (fix, not issue)

krvikash · 2023-03-24T08:06:00Z

plugin/trino-delta-lake/src/test/java/io/trino/plugin/deltalake/TestDeltaLakeBasic.java

+        assertQueryReturnsEmptyResult("SELECT column_name, data_type FROM information_schema.columns WHERE table_schema = CURRENT_SCHEMA AND table_name LIKE 'bad\\_perso_' ESCAPE '\\'");
+        assertQueryReturnsEmptyResult("SELECT column_name, data_type FROM system.jdbc.columns WHERE table_cat = CURRENT_CATALOG AND table_schem = CURRENT_SCHEMA AND table_name LIKE 'bad\\_perso_' ESCAPE '\\'");
+
+        // DROP TABLE should succeed so that users can remove their corrupted table
        getQueryRunner().execute("DROP TABLE " + tableName);
        assertFalse(getQueryRunner().tableExists(getSession(), tableName));


Can we also verify that the table location does not have any files after DROP TABLE execute?

as discussed offline, in this test we already deleted all the files

// break the table by deleting all its files including transaction log deleteRecursively(tableLocation, ALLOW_INSECURE);

i like the idea to improve the test. let's delete only transaction log and then add assertion that data files got removed.
since the PR is otherwise ready to go let's handle this as a follow-up

Follow up PR #16764

findepi · 2023-03-24T09:05:58Z

CI #7535 (will address with #16707)

findepi requested review from losipiuk, ebyhr, alexjo2144, krvikash and findinpath March 21, 2023 16:22

cla-bot bot added the cla-signed label Mar 21, 2023

findepi commented Mar 21, 2023

View reviewed changes

findepi force-pushed the findepi/drop-delta-corrupted branch from bb00968 to 9afbe9f Compare March 21, 2023 16:26

github-actions bot added the delta-lake Delta Lake connector label Mar 21, 2023

ebyhr approved these changes Mar 22, 2023

View reviewed changes

krvikash reviewed Mar 22, 2023

View reviewed changes

plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeMetadata.java Show resolved Hide resolved

findepi added 3 commits March 22, 2023 17:29

Access fields directly in DeltaLakeTableHandle.withProjectedColumns

809b5e6

This makes it consistent with other methods like that, in particular with `DeltaLakeTableHandle.forOptimize`.

Provide schema name in exception when Delta table lacks metadata

663d50c

Test Delta connector behavior for a corrupted table

5b178ea

findepi force-pushed the findepi/drop-delta-corrupted branch from ab8404e to 6045f41 Compare March 22, 2023 16:30

krvikash mentioned this pull request Mar 22, 2023

Support Iceberg's DROP TABLE for corrupted tables #16674

Merged

losipiuk reviewed Mar 23, 2023

View reviewed changes

losipiuk approved these changes Mar 23, 2023

View reviewed changes

findepi force-pushed the findepi/drop-delta-corrupted branch from 6045f41 to c23ec5e Compare March 23, 2023 12:18

krvikash reviewed Mar 24, 2023

View reviewed changes

findepi merged commit d2534ab into master Mar 24, 2023

findepi deleted the findepi/drop-delta-corrupted branch March 24, 2023 10:14

github-actions bot added this to the 411 milestone Mar 24, 2023

This was referenced Mar 25, 2023

Support drop iceberg table when metadata or snapshot is missing and refactor code for delta-lake #15065

Closed

Enhance test for managed and external delta table location validation #16764

Merged

colebow mentioned this pull request Mar 29, 2023

Add Trino 411 release notes #16552

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Delta's DROP TABLE support for corrupted tables #16651

Refactor Delta's DROP TABLE support for corrupted tables #16651

findepi commented Mar 21, 2023

findepi Mar 21, 2023

ebyhr Mar 22, 2023

findepi Mar 22, 2023

findepi Mar 21, 2023

findepi Mar 21, 2023

findepi Mar 21, 2023

findepi commented Mar 21, 2023

findepi commented Mar 21, 2023

ebyhr commented Mar 21, 2023

ebyhr Mar 22, 2023

findepi commented Mar 22, 2023

losipiuk Mar 23, 2023

findepi Mar 23, 2023

findepi commented Mar 23, 2023

krvikash Mar 24, 2023

findepi Mar 24, 2023

krvikash Mar 28, 2023

findepi commented Mar 24, 2023

Refactor Delta's DROP TABLE support for corrupted tables #16651

Refactor Delta's DROP TABLE support for corrupted tables #16651

Conversation

findepi commented Mar 21, 2023

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

findepi commented Mar 21, 2023

findepi commented Mar 21, 2023

ebyhr commented Mar 21, 2023

Choose a reason for hiding this comment

findepi commented Mar 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

findepi commented Mar 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

findepi commented Mar 24, 2023