Test Delta Lake metadata handling when schema already exists #15328

pajaks · 2022-12-07T12:08:02Z

Description

Add test for #13242 which was merged without a test as we didn't have an infrastructure for such tests at that time.

Additional context and related issues

Release notes

(x) This is not user-visible or docs only and no release notes are required.

cla-bot · 2022-12-07T12:08:04Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

findinpath · 2022-12-07T13:30:40Z

...-lake/src/test/java/io/trino/plugin/deltalake/metastore/glue/TestDeltaLakeGlueMetastore.java

+    }
+
+    @Test
+    public void testFailWhenCreateTheSameSchemaFromTwoSessions()


testFailWhenCreateTheSameSchemaFromTwoSessions -> testCreateSameSchemaFromDifferentSessionsFails

findinpath · 2022-12-07T13:35:08Z

...-lake/src/test/java/io/trino/plugin/deltalake/metastore/glue/TestDeltaLakeGlueMetastore.java

+        DeltaLakeMetadata metadata = metadataFactory.create(session.getIdentity());
+
+        metadata.createSchema(session, testSchemaName, Map.of(), TRINO_PRINCIPAL);
+        metadata.createSchema(session, testSchemaName, Map.of(), TRINO_PRINCIPAL);


Maybe call metadata.schemaExists() after the second call to be clear that the schema exists before doing the second call.

findinpath · 2022-12-07T13:35:34Z

...-lake/src/test/java/io/trino/plugin/deltalake/metastore/glue/TestDeltaLakeGlueMetastore.java

@@ -241,6 +252,37 @@ public void testHideNonDeltaLakeTable()
                .isEmpty();
    }

+    @Test
+    public void testDoubleCreateSchemaForTheSameSession()


testDoubleCreateSchemaForTheSameSession -> testCreateSchemaTwiceInTheSameSession

findinpath · 2022-12-07T13:37:35Z

...-lake/src/test/java/io/trino/plugin/deltalake/metastore/glue/TestDeltaLakeGlueMetastore.java

+        String testSchemaName = "test_schema_" + randomName();
+
+        DeltaLakeMetadata metadata = metadataFactory.create(session.getIdentity());
+


try/finally to ensure that the Glue database does get deleted even in case of exceptions

homar · 2022-12-07T13:50:01Z

...-lake/src/test/java/io/trino/plugin/deltalake/metastore/glue/TestDeltaLakeGlueMetastore.java

+        DeltaLakeMetadata metadata = metadataFactory.create(session.getIdentity());
+
+        metadata.createSchema(session, testSchemaName, Map.of(), TRINO_PRINCIPAL);
+        metadata.createSchema(session, testSchemaName, Map.of(), TRINO_PRINCIPAL);


In this and the second test case shouldn't we validate that schema was actually created?

findinpath · 2022-12-07T13:50:15Z

Commit message Add tests for creating schema

Maybe Test Delta AWS Glue handling when schema already exists

ebyhr

~~The new test testDoubleCreateSchemaForTheSameSession should fail without commits in #15177. Currently, it passes. I would recommend reverting commits temporarily when writing the test.~~ I realized the issue number in the description was wrong.

...-lake/src/test/java/io/trino/plugin/deltalake/metastore/glue/TestDeltaLakeGlueMetastore.java

pajaks · 2022-12-08T07:42:46Z

It was mistake in description. Tests are for #13242

ebyhr · 2022-12-08T08:04:04Z

Tests are for #13242

The PR isn't specific to Glue. Could you explain why this PR add tests only for Glue?

cla-bot · 2022-12-08T08:26:25Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

cla-bot · 2022-12-09T07:22:23Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

pajaks · 2022-12-09T07:59:26Z

Tests are for #13242

The PR isn't specific to Glue. Could you explain why this PR add tests only for Glue?

Tests were in wrong place. I moved them to generic tests and changed title.

findinpath · 2022-12-09T09:00:01Z

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

@@ -465,6 +473,35 @@ public void testGetInputInfoForUnPartitionedTable()
        assertThat(deltaLakeMetadata.getInfo(tableHandle)).isEqualTo(Optional.of(new DeltaLakeInputInfo(false)));
    }

+    @Test
+    public void testCreateSchemaTwiceInTheSameSession()


This feels like a unit test and somebody without a context for the SchemaAlreadyExistsException problem would have a hard time to figure out what are the tests there.
What about using a specific test - similar to TestIcebergFileMetastoreTableOperationsInsertFailure ?

cla-bot · 2022-12-12T11:54:29Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

ebyhr

Almost good to me. Thanks for adding this test.

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

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

cla-bot · 2022-12-13T09:21:11Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

...lta-lake/src/test/java/io/trino/plugin/deltalake/TestDeltaLakeSchemaCreateInternalRetry.java

cla-bot · 2022-12-13T10:36:00Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

ebyhr · 2022-12-14T00:12:50Z

Add tests for creating schema

@pajaks Could you fix the commit title as "Add tests for internal retry to create schema in Delta Lake"?

cla-bot · 2022-12-14T06:51:56Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

findinpath · 2022-12-14T10:04:35Z

...lta-lake/src/test/java/io/trino/plugin/deltalake/TestDeltaLakeCreateSchemaInternalRetry.java

+                            .build();
+                }
+                // Simulate retry mechanism with timeout failure.
+                // 1. createDatabase correctly create schema but timeout is triggered


createDatabase correctly create schema but timeout is triggered

nit:
createDatabase correctly creates schema, but timeout is triggered

findinpath · 2022-12-14T10:11:23Z

...lta-lake/src/test/java/io/trino/plugin/deltalake/TestDeltaLakeCreateSchemaInternalRetry.java

+            @Override
+            public synchronized void createDatabase(Database database)
+            {
+                if (database.getDatabaseName().equals(TEST_SCHEMA_DIFFERENT_SESSION)) {


Add this logic in a different test class to avoid mixing concerns and having the database.getDatabaseName().equals(TEST_SCHEMA_DIFFERENT_SESSION) check

The separated test class is overkill to me.

ebyhr · 2022-12-20T05:49:39Z

@cla-bot check

cla-bot · 2022-12-20T05:49:46Z

The cla-bot has been summoned, and re-checked this pull request!

ebyhr · 2022-12-21T07:59:09Z

@pajaks Are you going to add tests for Hive as well?

pajaks · 2022-12-21T08:03:25Z

@pajaks Are you going to add tests for Hive as well?

It seems like a good idea. I will look into it.

pajaks requested review from findepi, findinpath and homar December 7, 2022 12:10

findinpath reviewed Dec 7, 2022

View reviewed changes

homar reviewed Dec 7, 2022

View reviewed changes

ebyhr changed the title ~~Add tests for creating schema~~ Test Delta AWS Glue handling when schema already exists Dec 8, 2022

ebyhr reviewed Dec 8, 2022

View reviewed changes

...-lake/src/test/java/io/trino/plugin/deltalake/metastore/glue/TestDeltaLakeGlueMetastore.java Outdated Show resolved Hide resolved

pajaks changed the title ~~Test Delta AWS Glue handling when schema already exists~~ Test Delta Lake metadata handling when schema already exists Dec 9, 2022

findinpath reviewed Dec 9, 2022

View reviewed changes

ebyhr reviewed Dec 12, 2022

View reviewed changes

ebyhr approved these changes Dec 13, 2022

View reviewed changes

ebyhr reviewed Dec 13, 2022

View reviewed changes

...lta-lake/src/test/java/io/trino/plugin/deltalake/TestDeltaLakeSchemaCreateInternalRetry.java Outdated Show resolved Hide resolved

findinpath reviewed Dec 14, 2022

View reviewed changes

findinpath approved these changes Dec 14, 2022

View reviewed changes

martint force-pushed the master branch from 40dbb4f to 0d73d10 Compare December 19, 2022 20:15

cla-bot bot added the cla-signed label Dec 20, 2022

Add tests for internal retry to create schema in Delta Lake

df876da

ebyhr merged commit f1e7ba2 into trinodb:master Dec 20, 2022

github-actions bot added this to the 404 milestone Dec 20, 2022

ebyhr mentioned this pull request Dec 21, 2022

Add test for Schema already exists error when creating a schema in hive #15282

Closed

colebow mentioned this pull request Dec 21, 2022

Add Trino 405 release notes #15139

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test Delta Lake metadata handling when schema already exists #15328

Test Delta Lake metadata handling when schema already exists #15328

pajaks commented Dec 7, 2022 •

edited

Loading

cla-bot bot commented Dec 7, 2022

findinpath Dec 7, 2022

findinpath Dec 7, 2022

findinpath Dec 7, 2022

findinpath Dec 7, 2022

homar Dec 7, 2022

findinpath commented Dec 7, 2022

ebyhr left a comment •

edited

Loading

pajaks commented Dec 8, 2022

ebyhr commented Dec 8, 2022

cla-bot bot commented Dec 8, 2022

cla-bot bot commented Dec 9, 2022

pajaks commented Dec 9, 2022 •

edited

Loading

findinpath Dec 9, 2022

cla-bot bot commented Dec 12, 2022

ebyhr left a comment

cla-bot bot commented Dec 13, 2022

cla-bot bot commented Dec 13, 2022

ebyhr commented Dec 14, 2022

cla-bot bot commented Dec 14, 2022

findinpath Dec 14, 2022

findinpath Dec 14, 2022

ebyhr Dec 14, 2022

ebyhr commented Dec 20, 2022

cla-bot bot commented Dec 20, 2022

ebyhr commented Dec 21, 2022

pajaks commented Dec 21, 2022

		String testSchemaName = "test_schema_" + randomName();

		DeltaLakeMetadata metadata = metadataFactory.create(session.getIdentity());

Test Delta Lake metadata handling when schema already exists #15328

Test Delta Lake metadata handling when schema already exists #15328

Conversation

pajaks commented Dec 7, 2022 • edited Loading

Description

Additional context and related issues

Release notes

cla-bot bot commented Dec 7, 2022

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

findinpath commented Dec 7, 2022

ebyhr left a comment • edited Loading

Choose a reason for hiding this comment

pajaks commented Dec 8, 2022

ebyhr commented Dec 8, 2022

cla-bot bot commented Dec 8, 2022

cla-bot bot commented Dec 9, 2022

pajaks commented Dec 9, 2022 • edited Loading

Choose a reason for hiding this comment

cla-bot bot commented Dec 12, 2022

ebyhr left a comment

Choose a reason for hiding this comment

cla-bot bot commented Dec 13, 2022

cla-bot bot commented Dec 13, 2022

ebyhr commented Dec 14, 2022

cla-bot bot commented Dec 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebyhr commented Dec 20, 2022

cla-bot bot commented Dec 20, 2022

ebyhr commented Dec 21, 2022

pajaks commented Dec 21, 2022

pajaks commented Dec 7, 2022 •

edited

Loading

ebyhr left a comment •

edited

Loading

pajaks commented Dec 9, 2022 •

edited

Loading