Support Glue catalog in Iceberg connector by jackye1995 · Pull Request #10151 · trinodb/trino

jackye1995 · 2021-12-02T06:40:44Z

Add support for reading Glue catalog data and creating Glue table, based on draft in #9646.

Also reorganized the files in the following ways:

moved catalog related classes to the catalog module
changed TrinoCatalogFactory to an interface to work with dependency injection
split some logic in AbstractMetastoreTableOperations to AbstractIcebergTableOperations to share with Glue implementation
add catalog config iceberg.default-schema-location as discussed in Support Iceberg default warehouse location config #9614
add test profile test-iceberg-glue to not run the Glue test because it requires AWS setup. I have ran the test to make sure all 280 tests pass.

jackye1995 · 2021-12-02T06:42:26Z

@findepi @losipiuk @electrum

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

findepi · 2021-12-02T08:44:17Z

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

This could probably use BaseConnectorSmokeTest.

Also, a test class should not extend from a test class.
For code sharing, an explicit abstract Base.. class should be used

changed to use BaseIcebergConnectorTest

sorry did not read the first line, changed to smoke test instead

findepi · 2021-12-02T08:44:54Z

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

Suggested change

Map.of("iceberg.file-format", "parquet",

Map.of(

"iceberg.file-format", "parquet",

findepi · 2021-12-02T08:45:22Z

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

Trino Glue catalog -> Iceberg Glue catalog

findepi · 2021-12-02T08:45:37Z

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

why override? document

because Glue has no database user concept and thus does not support AUTHORIZATION USER

the test in smoke test works, removed

jackye1995 · 2021-12-10T08:24:28Z

@findepi Regarding running the Glue test, I checked Hive does run the Glue test suite using:

      - name: Run Hive Glue Tests
        env:
          AWS_ACCESS_KEY_ID: ${{ secrets.AWS_ACCESSKEY }}
          AWS_SECRET_ACCESS_KEY: ${{ secrets.AWS_SECRETKEY }}
          AWS_REGION: us-east-2
        run: |
          if [ "${AWS_ACCESS_KEY_ID}" != "" ]; then
            $MAVEN test ${MAVEN_TEST} -pl :trino-hive -P test-hive-glue
          fi

But hive-tests is the only one that does this, Iceberg is in test-other-modules. I think it's probably cleaner to create a new suite dedicated for iceberg-test, please let me know if that's the right way to update CI, or if there is any other way preferred, thanks.

findepi · 2021-12-16T11:07:17Z

plugin/trino-iceberg/pom.xml

+        </profile>
+
+        <profile>
+            <id>test-iceberg-glue</id>


This needs to be run on CI.

Maybe instead of a profile with single test, let's have a profile with all tests (no exclusions).

trino/.github/workflows/ci.yml

Line 351 in 3c0cf0b

- ":trino-iceberg,:trino-druid"

would become

- ":trino-iceberg -P test-all,:trino-druid"

also, we should move trino-druid to a different group, maybe with kudu (need to check running times)

findepi · 2021-12-16T11:09:37Z

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

    ICEBERG_WRITE_VALIDATION_FAILED(10, INTERNAL_ERROR),
    ICEBERG_INVALID_SNAPSHOT_ID(11, USER_ERROR),
+    ICEBERG_CATALOG_ERROR(12, EXTERNAL),
+    ICEBERG_COMMIT_ERROR(13, EXTERNAL)


Extract commit introducing this error code.
It should be used in io.trino.plugin.iceberg.catalog.hms.AbstractMetastoreTableOperations#commitNewTable and io.trino.plugin.iceberg.catalog.hms.HiveMetastoreTableOperations#commitToExistingTable

findepi · 2021-12-16T11:25:59Z

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

+        return defaultSchemaLocation;
+    }
+
+    @Config("iceberg.default-schema-location")


This is applicable to Glue only, so should be in a config class that's bound only when Glue catalog is used.
it should start with iceberg.glue.

findepi · 2021-12-16T11:26:43Z

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

 import io.trino.plugin.hive.gcs.HiveGcsModule;
 import io.trino.plugin.hive.metastore.HiveMetastore;
 import io.trino.plugin.hive.s3.HiveS3Module;
+import io.trino.plugin.iceberg.catalog.IcebergCatalogModule;


Extract commit that introduces io.trino.plugin.iceberg.catalog package.

findepi · 2021-12-16T11:31:31Z

...no-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/AbstractIcebergTableOperations.java


 @NotThreadSafe
-public abstract class AbstractMetastoreTableOperations
+public abstract class AbstractIcebergTableOperations


Extract a commit which splits AbstractMetastoreTableOperations into AbstractMetastoreTableOperations and AbstractIcebergTableOperations

findepi · 2021-12-16T13:05:15Z

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

+            throw new TrinoException(ICEBERG_COMMIT_ERROR, format("Cannot commit %s due to unexpected exception", getSchemaTableName()), e);
+        }
+        finally {
+            cleanupMetadataLocation(!succeeded, newMetadataLocation);


this can swallow exception in flight.

findepi · 2021-12-16T13:05:25Z

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

+            throw new TrinoException(ICEBERG_COMMIT_ERROR, format("Cannot commit %s because of concurrent update", getSchemaTableName()), e);
+        }
+        finally {
+            cleanupMetadataLocation(!succeeded, newMetadataLocation);


this can swallow exception in flight.

findepi · 2021-12-16T13:06:09Z

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

+            try {
+                io().deleteFile(metadataLocation);
+            }
+            catch (RuntimeException ex) {


findepi · 2021-12-16T13:08:21Z

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

+        return createIcebergQueryRunner(
+                ImmutableMap.of(),
+                ImmutableMap.of(
+                        "iceberg.file-format", "orc",


findepi · 2021-12-16T13:08:42Z

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

+    protected boolean hasBehavior(TestingConnectorBehavior connectorBehavior)
+    {
+        switch (connectorBehavior) {
+            case SUPPORTS_RENAME_SCHEMA:
+            case SUPPORTS_COMMENT_ON_COLUMN:
+            case SUPPORTS_TOPN_PUSHDOWN:
+            case SUPPORTS_CREATE_VIEW:
+            case SUPPORTS_CREATE_MATERIALIZED_VIEW:
+            case SUPPORTS_RENAME_MATERIALIZED_VIEW:
+            case SUPPORTS_RENAME_MATERIALIZED_VIEW_ACROSS_SCHEMAS:
+                return false;
+
+            case SUPPORTS_DELETE:
+                return true;
+            default:
+                return super.hasBehavior(connectorBehavior);
+        }


This class and TestIcebergConnectorSmokeTest should have a common base (BaseIcebergConnectorSmokeTest) capturing Iceberg behavior.

findepi · 2021-12-16T13:09:59Z

cc @phd3

alexjo2144 · 2022-01-24T21:21:24Z

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

+import static org.apache.iceberg.BaseMetastoreTableOperations.ICEBERG_TABLE_TYPE_VALUE;
+import static org.apache.iceberg.BaseMetastoreTableOperations.TABLE_TYPE_PROP;
+
+public class GlueTableOperations


GlueIcebergTableOperations, seems more natural from the base class AbstractIcebergTableOperations

alexjo2144 · 2022-01-24T21:21:58Z

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

+    @Override
+    protected String getRefreshedLocation()
+    {
+        return stats.getGetTable().call(() -> {


Just wrap the Glue API call in the lambda

alexjo2144 · 2022-01-24T21:32:32Z

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

+    @Inject
+    public GlueTableOperationsProvider(FileIoProvider fileIoProvider)
+    {
+        this.fileIoProvider = fileIoProvider;


requireNonNull

alexjo2144 · 2022-01-24T21:39:22Z

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

+    private final String catalogId;
+    private final GlueMetastoreStats stats;
+
+    private final Map<SchemaTableName, TableMetadata> tableMetadataCache = new ConcurrentHashMap<>();


How are entries in this cache invalidated?

alexjo2144 · 2022-01-24T21:53:13Z

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

+        this.tableOperationsProvider = requireNonNull(tableOperationsProvider, "tableOperationsProvider is null");
+        this.glueClient = requireNonNull(glueClient, "glueClient is null");
+        this.stats = requireNonNull(stats, "stats is null");
+        this.catalogId = catalogId; // null is a valid catalogId, meaning the current account


Add an @nullable annotation

alexjo2144 · 2022-01-24T21:58:15Z

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

+    @Override
+    public void renameNamespace(ConnectorSession session, String source, String target)
+    {
+        throw new TrinoException(NOT_SUPPORTED, "renameNamespace is not supported by Iceberg Glue catalog");


Error message nit:

Suggested change

throw new TrinoException(NOT_SUPPORTED, "renameNamespace is not supported by Iceberg Glue catalog");

throw new TrinoException(NOT_SUPPORTED, "renameNamespace is not supported for Iceberg Glue catalogs");

findepi · 2022-02-07T16:19:16Z

Superseded by #10845

cla-bot bot added the cla-signed label Dec 2, 2021

jackye1995 mentioned this pull request Dec 2, 2021

Support Iceberg default warehouse location config #9614

Closed

jackye1995 force-pushed the glue-catalog branch from e84aab7 to 59d9dff Compare December 2, 2021 06:44

jackye1995 mentioned this pull request Dec 2, 2021

Add Iceberg Glue and Hadoop catalog support #9646

Closed

findepi reviewed Dec 2, 2021

View reviewed changes

jackye1995 force-pushed the glue-catalog branch 2 times, most recently from b9feee2 to 3ac1f4d Compare December 9, 2021 07:34

jackye1995 requested a review from findepi December 9, 2021 07:34

jackye1995 force-pushed the glue-catalog branch 2 times, most recently from 523fede to 4ef57e9 Compare December 9, 2021 18:50

Support Glue catalog in Iceberg connector

0541b2d

jackye1995 force-pushed the glue-catalog branch from 4ef57e9 to 0541b2d Compare December 9, 2021 19:04

jackye1995 requested a review from alexjo2144 December 9, 2021 19:04

jackye1995 requested a review from losipiuk December 10, 2021 18:42

findepi reviewed Dec 16, 2021

View reviewed changes

findepi mentioned this pull request Dec 17, 2021

Disallow dropping Iceberg schema that contains external files #9767

Closed

alexjo2144 reviewed Jan 24, 2022

View reviewed changes

alexjo2144 mentioned this pull request Jan 28, 2022

Support Glue catalog in Iceberg connector #10845

Merged

findepi closed this Feb 7, 2022

	Map.of("iceberg.file-format", "parquet",
	Map.of(
	"iceberg.file-format", "parquet",

	throw new TrinoException(NOT_SUPPORTED, "renameNamespace is not supported by Iceberg Glue catalog");
	throw new TrinoException(NOT_SUPPORTED, "renameNamespace is not supported for Iceberg Glue catalogs");

Conversation

jackye1995 commented Dec 2, 2021

Uh oh!

jackye1995 commented Dec 2, 2021

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!

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!

jackye1995 commented Dec 10, 2021

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!

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!

findepi commented Dec 16, 2021

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!

Choose a reason for hiding this comment

Uh oh!

findepi commented Feb 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants