Support drop iceberg table when metadata or snapshot is missing and refactor code for delta-lake by krvikash · Pull Request #15065 · trinodb/trino

krvikash · 2022-11-16T21:56:48Z

Description

This PR Includes below:

SPI:
Introduced new SPI method dropTable which takes SchemaTableName. It is the connector's responsibility to implement the method.

Note: dropTable with table handle deprecation/removal will be taken care in new PR

Iceberg connector:

When the latest manifest file does not exist --> Fails to drop table (Fixed)
When snapshot file does not exist --> drops table in metastore, But the fails to delete data and metadata files (Fixed)
When ManifestList file does not exist --> No Fix required, Already working
When data file does not exist --> No Fix required, Already working

Q: What if latest manifest file or snapshot file does not exist and data files referenced by TableMetadata located in different locations?

Delta-Lake connector:

Consumed new SPI method dropTable with SchemaTableName in case delta log does not exist
Made metadataEntry non-optional in DeltaLakeTableHandle

Non-technical explanation

NA

Release notes

( ) This is not user-visible or docs only and no release notes are required.
(X) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

core/trino-main/src/main/java/io/trino/metadata/Metadata.java

core/trino-main/src/main/java/io/trino/metadata/MetadataManager.java

core/trino-main/src/main/java/io/trino/testing/TestingMetadata.java

core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorMetadata.java

findinpath · 2022-11-17T09:45:07Z

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/glue/TrinoGlueCatalog.java

Why is the exception being swallowed?

Because we do not want to fail when FileNotFoundException exception occurs instead we want to drop the table from metastore and delete table location directory.

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

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

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

core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorMetadata.java

alexjo2144 · 2022-11-17T17:55:33Z

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

I'm not positive this will always be a FileNotFoundException. If you're using S3 for example it's probably something like an AmazonServiceException with an error code "Specified key does not exist"

Modified the exception catch to handle S3 exception. Verified GCP and ADLS exceptions as well.

alexjo2144 · 2022-11-17T17:59:04Z

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

I think we should still validate that the table exists here, and throw a TableNotFound exception if it doesn't

Respective catalog call will take care of validating the table exists.

core/trino-spi/src/main/java/io/trino/spi/StandardErrorCode.java

core/trino-main/src/main/java/io/trino/execution/DropTableTask.java

ebyhr · 2022-11-18T06:24:54Z

Could you update PR title to reflect Delta Lake connector?

findepi · 2022-11-18T09:24:14Z

core/trino-main/src/main/java/io/trino/metadata/Metadata.java

We want this method to replace the old one, void dropTable(Session session, TableHandle tableHandle).
So it would be good to

make ConnectorMetadata change backward compatibility

apply changes to DropTableTask

Doing this would reveal that the old drop handles redirections whereas the new drop-by-name cannot do that.
The MetadataManager could follow redirects as needed, but I am concerned this is not the right place to add that logic.

Also, decoupling ConnectorMetadata.redirectTable and ConnectorMetadata.dropTable calls makes it impossible to make a non-racy implementation.
I am leaving towards introducing a non-void return type here, so that drop-by-name can either declare success or return a redirection.

@martint wdyt?

This looks unresolved. @krvikash @findinpath do you have a plan here?

perhaps something like this (rough idea, not polished up)

DropResult dropTable(Session session, QualifiedObjectName tableName);

sealed class DropResult permits DropSuccess, DropRedirected

class DropSuccess { .. } // singleton (can this be an enum?)

records DropRedirected(CatalogSchemaTableName target) {}

krvikash · 2022-11-18T13:58:03Z

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/hms/TrinoHiveCatalog.java

When the metadata file is not found, Then we won't be able to get the table properties and validate if the table can be dropped. How to handle this?

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

core/trino-main/src/main/java/io/trino/metadata/MetadataManager.java

findepi · 2022-12-06T12:00:45Z

core/trino-main/src/main/java/io/trino/execution/DropTableTask.java

why is this exception-driven?
what potential exceptions are we ignoring here?

I want to check the exception which fails with the METADATA_NOT_FOUND error.

We should probably rethink here the way we obtain the tableName. We don't need actually the full tableHandle for dropping the table, right?

Swallowing the exception gets us to the state of adding the DropResult , DropRedirected and DropSuccess classes and extra logic in MetadataManager.

I think the approach to solve this issue should be thought over.

I want to check the exception which fails with the METADATA_NOT_FOUND error.

metadata.dropTable must support such case anyway
what does it currently throw?

core/trino-main/src/main/java/io/trino/execution/DropTableTask.java

findepi · 2022-12-06T12:02:29Z

core/trino-main/src/main/java/io/trino/metadata/Metadata.java

This looks unresolved. @krvikash @findinpath do you have a plan here?

core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorMetadata.java

core/trino-spi/src/main/java/io/trino/spi/StandardErrorCode.java

findepi

"Introduce dropTable SPI method that takes SchemaTableName"

DropTableTask lgtm

please clean up the commit (deprecation, unused class)

krvikash · 2023-01-03T14:37:47Z

I have written a doc for the approaches we have taken for this PR https://docs.google.com/document/d/1uydrtqfj4S5dhyrQ8JqAsW3fqr1ACXmEvj9MYSpJpLw/

CC: @findepi

findepi · 2023-02-07T16:36:05Z

sorry for not following up earlier on this.
I see the build is quite totally red. do you want to fix it before my re-review?

krvikash · 2023-02-08T07:31:45Z

Rebased and resolved the conflicts.

@findepi, I am using using Approach 3 from https://docs.google.com/document/d/1uydrtqfj4S5dhyrQ8JqAsW3fqr1ACXmEvj9MYSpJpLw/ which is not working and test cases will fail. So need to finalize what approach we can take to make it work.

krvikash · 2023-02-13T09:14:01Z

Rebased

krvikash · 2023-03-06T09:05:42Z

Rebased with the master and resolve conflicts.

findepi · 2023-03-21T09:58:59Z

(rebased resolving a conflict)

findepi · 2023-03-21T10:01:11Z

core/trino-main/src/main/java/io/trino/metadata/Metadata.java

     *
     * @throws RuntimeException if the table cannot be dropped or table handle is no longer valid
+     *
+     * @deprecated use {@link #dropTable(Session, QualifiedObjectName)}


This overload is a dead code now, no need for keeping it (as deprecated)

findepi · 2023-03-21T13:13:46Z

...no-product-tests/src/main/java/io/trino/tests/product/hive/TestHiveRedirectionToIceberg.java

    }

+    @Test(groups = {HIVE_ICEBERG_REDIRECTIONS, PROFILE_SPECIFIC_TESTS})
+    public void testDropTableWithMissingMetadataFile()


Would it make sense to have it tested outside of PTs?

krvikash · 2023-03-25T07:12:45Z

Closing in favor of #16674 and #16651

cla-bot bot added the cla-signed label Nov 16, 2022

krvikash changed the title ~~Support drop corrupted iceberg table~~ Support drop iceberg table when metadata or snapshot is missing Nov 16, 2022

krvikash force-pushed the support-drop-corrupted-iceberg-table branch from bd6d342 to 604b6c9 Compare November 17, 2022 06:59

krvikash marked this pull request as ready for review November 17, 2022 07:01

krvikash requested review from alexjo2144, ebyhr, findepi and findinpath November 17, 2022 07:01

krvikash force-pushed the support-drop-corrupted-iceberg-table branch from 604b6c9 to 2ab0747 Compare November 17, 2022 07:17

findinpath reviewed Nov 17, 2022

View reviewed changes

krvikash force-pushed the support-drop-corrupted-iceberg-table branch 4 times, most recently from 4234827 to eedddc1 Compare November 17, 2022 13:24

github-actions bot added the tests:hive label Nov 17, 2022

alexjo2144 reviewed Nov 17, 2022

View reviewed changes

krvikash changed the title ~~Support drop iceberg table when metadata or snapshot is missing~~ Support drop iceberg table when metadata or snapshot is missing and refactor code for delta-lake Nov 18, 2022

findepi reviewed Nov 18, 2022

View reviewed changes

findepi requested review from martint and phd3 November 18, 2022 09:24

krvikash commented Nov 18, 2022

View reviewed changes

krvikash force-pushed the support-drop-corrupted-iceberg-table branch 2 times, most recently from 7dc64a3 to 649ccd0 Compare November 18, 2022 23:40

krvikash commented Nov 18, 2022

View reviewed changes

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java Outdated Show resolved Hide resolved

krvikash force-pushed the support-drop-corrupted-iceberg-table branch 2 times, most recently from da95d64 to 62b2221 Compare November 19, 2022 18:35

krvikash force-pushed the support-drop-corrupted-iceberg-table branch 2 times, most recently from 7cc24be to 7375027 Compare December 5, 2022 12:14

findepi reviewed Dec 6, 2022

View reviewed changes

krvikash mentioned this pull request Dec 20, 2022

Add test for DROP TABLE IF EXISTS statement with non-existing object #15464

Merged

krvikash force-pushed the support-drop-corrupted-iceberg-table branch from e127e6e to 038d833 Compare December 20, 2022 14:46

findepi reviewed Dec 21, 2022

View reviewed changes

core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorMetadata.java Outdated Show resolved Hide resolved

findepi reviewed Dec 21, 2022

View reviewed changes

core/trino-spi/src/main/java/io/trino/spi/StandardErrorCode.java Outdated Show resolved Hide resolved

findepi reviewed Dec 21, 2022

View reviewed changes

findepi mentioned this pull request Dec 21, 2022

[Alternative] Support drop corrupted iceberg table #15435

Closed

krvikash force-pushed the support-drop-corrupted-iceberg-table branch from 038d833 to bf2bc50 Compare December 21, 2022 18:51

krvikash force-pushed the support-drop-corrupted-iceberg-table branch from bf2bc50 to 2860eee Compare February 8, 2023 07:27

krvikash force-pushed the support-drop-corrupted-iceberg-table branch from 2860eee to f83da0c Compare February 13, 2023 09:13

krvikash requested a review from findepi March 6, 2023 08:50

krvikash force-pushed the support-drop-corrupted-iceberg-table branch from f83da0c to b07a719 Compare March 6, 2023 09:05

github-actions bot added delta-lake Delta Lake connector hive Hive connector iceberg Iceberg connector labels Mar 6, 2023

kokosing force-pushed the master branch from 3f05134 to 58d6356 Compare March 14, 2023 11:34

krvikash added 4 commits March 21, 2023 10:58

Introduce dropTable SPI method that takes SchemaTableName

3550a3c

Support drop iceberg table when metadata or snapshot file is missing

ec7ce17

Implement dropTable with SchemaTableName in DeltaLakeMetadata

376a0de

Make metadataEntry non-optional for DeltaLakeTableHandle

1856081

findepi force-pushed the support-drop-corrupted-iceberg-table branch from b07a719 to 1856081 Compare March 21, 2023 09:58

findepi reviewed Mar 21, 2023

View reviewed changes

krvikash closed this Mar 25, 2023

krvikash deleted the support-drop-corrupted-iceberg-table branch March 25, 2023 07:12

Conversation

krvikash commented Nov 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Non-technical explanation

Release notes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ebyhr commented Nov 18, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

findinpath Dec 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

findepi left a comment

Choose a reason for hiding this comment

Uh oh!

krvikash commented Jan 3, 2023

Uh oh!

findepi commented Feb 7, 2023

Uh oh!

krvikash commented Feb 8, 2023

Uh oh!

krvikash commented Feb 13, 2023

Uh oh!

krvikash commented Mar 6, 2023

Uh oh!

findepi commented Mar 21, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krvikash commented Mar 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

krvikash commented Nov 16, 2022 •

edited

Loading

findinpath Dec 16, 2022 •

edited

Loading

krvikash commented Mar 25, 2023 •

edited

Loading