From e8f637dc684157192bf61bc81eb8877ca49e7a7b Mon Sep 17 00:00:00 2001 From: Piotr Findeisen Date: Tue, 20 Jun 2023 17:34:10 +0200 Subject: [PATCH 1/8] Configure TestIcebergS3AndGlueMetastoreTest as other similar tests Configure bucket used in `TestIcebergS3AndGlueMetastoreTest` same way as in other `BaseS3AndGlueMetastoreTest` subclasses. Makes it easier to run tests locally. This also changes `TestIcebergGlueCatalogConnectorSmokeTest` for consistency. --- .github/workflows/ci.yml | 1 - .../glue/TestIcebergGlueCatalogConnectorSmokeTest.java | 6 ++---- .../catalog/glue/TestIcebergS3AndGlueMetastoreTest.java | 7 +++---- 3 files changed, 5 insertions(+), 9 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f1c7beee5cc2..6c32bdb7bc38 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -719,7 +719,6 @@ jobs: (env.CI_SKIP_SECRETS_PRESENCE_CHECKS != '' || env.AWS_ACCESS_KEY_ID != '' || env.AWS_SECRET_ACCESS_KEY != '' || env.GCP_CREDENTIALS_KEY != '') run: | $MAVEN test ${MAVEN_TEST} -pl :trino-iceberg ${{ format('-P {0}', matrix.profile) }} \ - -Ds3.bucket=${S3_BUCKET} \ -Dtesting.gcp-storage-bucket="${GCP_STORAGE_BUCKET}" \ -Dtesting.gcp-credentials-key="${GCP_CREDENTIALS_KEY}" \ -Dhive.hadoop2.azure-abfs-container="${ABFS_CONTAINER}" \ diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/glue/TestIcebergGlueCatalogConnectorSmokeTest.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/glue/TestIcebergGlueCatalogConnectorSmokeTest.java index 76daa56560ff..247be6409f18 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/glue/TestIcebergGlueCatalogConnectorSmokeTest.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/glue/TestIcebergGlueCatalogConnectorSmokeTest.java @@ -43,7 +43,6 @@ import io.trino.testing.QueryRunner; import org.apache.iceberg.FileFormat; import org.testng.annotations.AfterClass; -import org.testng.annotations.Parameters; import org.testng.annotations.Test; import java.util.List; @@ -72,11 +71,10 @@ public class TestIcebergGlueCatalogConnectorSmokeTest private final AWSGlueAsync glueClient; private final TrinoFileSystemFactory fileSystemFactory; - @Parameters("s3.bucket") - public TestIcebergGlueCatalogConnectorSmokeTest(String bucketName) + public TestIcebergGlueCatalogConnectorSmokeTest() { super(FileFormat.PARQUET); - this.bucketName = requireNonNull(bucketName, "bucketName is null"); + this.bucketName = requireNonNull(System.getenv("S3_BUCKET"), "Environment S3_BUCKET was not set"); this.schemaName = "test_iceberg_smoke_" + randomNameSuffix(); glueClient = AWSGlueAsyncClientBuilder.defaultClient(); diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/glue/TestIcebergS3AndGlueMetastoreTest.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/glue/TestIcebergS3AndGlueMetastoreTest.java index aae36ebc3095..84b9de648439 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/glue/TestIcebergS3AndGlueMetastoreTest.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/glue/TestIcebergS3AndGlueMetastoreTest.java @@ -19,7 +19,6 @@ import io.trino.plugin.iceberg.IcebergQueryRunner; import io.trino.testing.DistributedQueryRunner; import io.trino.testing.QueryRunner; -import org.testng.annotations.Parameters; import org.testng.annotations.Test; import java.nio.file.Path; @@ -28,16 +27,16 @@ import static io.trino.plugin.hive.metastore.glue.GlueHiveMetastore.createTestingGlueHiveMetastore; import static io.trino.testing.TestingNames.randomNameSuffix; +import static java.util.Objects.requireNonNull; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; public class TestIcebergS3AndGlueMetastoreTest extends BaseS3AndGlueMetastoreTest { - @Parameters("s3.bucket") - public TestIcebergS3AndGlueMetastoreTest(String bucketName) + public TestIcebergS3AndGlueMetastoreTest() { - super("partitioning", "location", bucketName); + super("partitioning", "location", requireNonNull(System.getenv("S3_BUCKET"), "Environment S3_BUCKET was not set")); } @Override From b34d8791c0823596035a50e391f121f7ca539f9c Mon Sep 17 00:00:00 2001 From: Piotr Findeisen Date: Tue, 20 Jun 2023 17:22:58 +0200 Subject: [PATCH 2/8] Do not prune file system cache This may be needed when working with MinIO containers where bucket names can be reused. This is not needed when working with real S3 where bucket names are unique. Also, it's unclear whether this is a safe operation when tests execute in parallel. --- .../metastore/glue/TestDeltaS3AndGlueMetastoreTest.java | 3 --- .../io/trino/plugin/hive/TestHiveS3AndGlueMetastoreTest.java | 3 --- .../catalog/glue/TestIcebergS3AndGlueMetastoreTest.java | 3 --- 3 files changed, 9 deletions(-) diff --git a/plugin/trino-delta-lake/src/test/java/io/trino/plugin/deltalake/metastore/glue/TestDeltaS3AndGlueMetastoreTest.java b/plugin/trino-delta-lake/src/test/java/io/trino/plugin/deltalake/metastore/glue/TestDeltaS3AndGlueMetastoreTest.java index 533b87b749c2..cc12d940b2ee 100644 --- a/plugin/trino-delta-lake/src/test/java/io/trino/plugin/deltalake/metastore/glue/TestDeltaS3AndGlueMetastoreTest.java +++ b/plugin/trino-delta-lake/src/test/java/io/trino/plugin/deltalake/metastore/glue/TestDeltaS3AndGlueMetastoreTest.java @@ -14,7 +14,6 @@ package io.trino.plugin.deltalake.metastore.glue; import com.google.common.collect.ImmutableMap; -import io.trino.hdfs.TrinoFileSystemCache; import io.trino.plugin.deltalake.DeltaLakeQueryRunner; import io.trino.plugin.hive.BaseS3AndGlueMetastoreTest; import io.trino.testing.DistributedQueryRunner; @@ -42,8 +41,6 @@ public TestDeltaS3AndGlueMetastoreTest() protected QueryRunner createQueryRunner() throws Exception { - closeAfterClass(TrinoFileSystemCache.INSTANCE::closeAll); - metastore = createTestingGlueHiveMetastore(Path.of(schemaPath())); DistributedQueryRunner queryRunner = DeltaLakeQueryRunner.builder() .setCatalogName(DELTA_CATALOG) diff --git a/plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHiveS3AndGlueMetastoreTest.java b/plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHiveS3AndGlueMetastoreTest.java index 4df2b89396ac..73b73df62531 100644 --- a/plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHiveS3AndGlueMetastoreTest.java +++ b/plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHiveS3AndGlueMetastoreTest.java @@ -15,7 +15,6 @@ import com.google.common.collect.ImmutableMap; import io.trino.Session; -import io.trino.hdfs.TrinoFileSystemCache; import io.trino.spi.security.Identity; import io.trino.spi.security.SelectedRole; import io.trino.testing.DistributedQueryRunner; @@ -47,8 +46,6 @@ public TestHiveS3AndGlueMetastoreTest() protected QueryRunner createQueryRunner() throws Exception { - closeAfterClass(TrinoFileSystemCache.INSTANCE::closeAll); - metastore = createTestingGlueHiveMetastore(Path.of(schemaPath())); Session session = createSession(Optional.of(new SelectedRole(ROLE, Optional.of("admin")))); diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/glue/TestIcebergS3AndGlueMetastoreTest.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/glue/TestIcebergS3AndGlueMetastoreTest.java index 84b9de648439..b45e52ef55ee 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/glue/TestIcebergS3AndGlueMetastoreTest.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/glue/TestIcebergS3AndGlueMetastoreTest.java @@ -14,7 +14,6 @@ package io.trino.plugin.iceberg.catalog.glue; import com.google.common.collect.ImmutableMap; -import io.trino.hdfs.TrinoFileSystemCache; import io.trino.plugin.hive.BaseS3AndGlueMetastoreTest; import io.trino.plugin.iceberg.IcebergQueryRunner; import io.trino.testing.DistributedQueryRunner; @@ -43,8 +42,6 @@ public TestIcebergS3AndGlueMetastoreTest() protected QueryRunner createQueryRunner() throws Exception { - closeAfterClass(TrinoFileSystemCache.INSTANCE::closeAll); - metastore = createTestingGlueHiveMetastore(Path.of(schemaPath())); DistributedQueryRunner queryRunner = IcebergQueryRunner.builder() .setIcebergProperties(ImmutableMap.builder() From 1e90f7865635b9207a30f34d62f95f1807eb1afc Mon Sep 17 00:00:00 2001 From: Piotr Findeisen Date: Tue, 20 Jun 2023 09:28:16 +0200 Subject: [PATCH 3/8] Code cleanup --- .../plugin/hive/BaseS3AndGlueMetastoreTest.java | 4 ++-- .../hive/TestHiveS3AndGlueMetastoreTest.java | 4 ++-- .../glue/TestIcebergS3AndGlueMetastoreTest.java | 16 +++++++--------- 3 files changed, 11 insertions(+), 13 deletions(-) diff --git a/plugin/trino-hive/src/test/java/io/trino/plugin/hive/BaseS3AndGlueMetastoreTest.java b/plugin/trino-hive/src/test/java/io/trino/plugin/hive/BaseS3AndGlueMetastoreTest.java index 130852a5368e..627a5c972a8e 100644 --- a/plugin/trino-hive/src/test/java/io/trino/plugin/hive/BaseS3AndGlueMetastoreTest.java +++ b/plugin/trino-hive/src/test/java/io/trino/plugin/hive/BaseS3AndGlueMetastoreTest.java @@ -250,9 +250,9 @@ protected void validateFilesAfterOptimize(String location, Set initialFi protected abstract void validateMetadataFiles(String location); - protected void validateTableLocation(String tableName, String location) + protected void validateTableLocation(String tableName, String expectedLocation) { - assertThat(getTableLocation(tableName)).isEqualTo(location); + assertThat(getTableLocation(tableName)).isEqualTo(expectedLocation); } protected void validateFilesAfterDrop(String location) diff --git a/plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHiveS3AndGlueMetastoreTest.java b/plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHiveS3AndGlueMetastoreTest.java index 73b73df62531..34cb9632b93f 100644 --- a/plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHiveS3AndGlueMetastoreTest.java +++ b/plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHiveS3AndGlueMetastoreTest.java @@ -212,7 +212,7 @@ public void testAnalyzeWithProvidedTableLocation(boolean partitioned, String loc assertUpdate("INSERT INTO " + tableName + " VALUES ('str4', 4)", 1); assertQuery("SELECT * FROM " + tableName, "VALUES ('str1', 1), ('str2', 2), ('str3', 3), ('str4', 4)"); - //Check statistics collection on write + // Check statistics collection on write if (partitioned) { assertQuery("SHOW STATS FOR " + tableName, """ VALUES @@ -228,7 +228,7 @@ public void testAnalyzeWithProvidedTableLocation(boolean partitioned, String loc (null, null, null, null, 4.0, null, null)"""); } - //Check statistics collection explicitly + // Check statistics collection explicitly assertUpdate("ANALYZE " + tableName, 4); if (partitioned) { diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/glue/TestIcebergS3AndGlueMetastoreTest.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/glue/TestIcebergS3AndGlueMetastoreTest.java index b45e52ef55ee..0de6bd41a363 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/glue/TestIcebergS3AndGlueMetastoreTest.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/glue/TestIcebergS3AndGlueMetastoreTest.java @@ -77,15 +77,13 @@ protected void validateMetadataFiles(String location) } @Override - protected void validateTableLocation(String tableName, String location) + protected void validateTableLocation(String tableName, String expectedLocation) { - if (location.endsWith("/")) { - //Iceberg removes trailing slash from location, and it's expected. - assertThat(getTableLocation(tableName) + "/").isEqualTo(location); - } - else { - assertThat(getTableLocation(tableName)).isEqualTo(location); + // Iceberg removes trailing slash from location, and it's expected. + if (expectedLocation.endsWith("/")) { + expectedLocation = expectedLocation.replaceFirst("/$", ""); } + assertThat(getTableLocation(tableName)).isEqualTo(expectedLocation); } private Set getAllMetadataDataFilesFromTableDirectory(String tableLocation) @@ -123,12 +121,12 @@ public void testAnalyzeWithProvidedTableLocation(boolean partitioned, String loc ('col_int', null, 4.0, 0.0, null, 1, 4), (null, null, null, null, 4.0, null, null)"""; - //Check extended statistics collection on write + // Check extended statistics collection on write assertQuery("SHOW STATS FOR " + tableName, expectedStatistics); // drop stats assertUpdate("ALTER TABLE " + tableName + " EXECUTE DROP_EXTENDED_STATS"); - //Check extended statistics collection explicitly + // Check extended statistics collection explicitly assertUpdate("ANALYZE " + tableName); assertQuery("SHOW STATS FOR " + tableName, expectedStatistics); From 3f34a99a507316736e6fcdc55f69696e0d06d8c9 Mon Sep 17 00:00:00 2001 From: Piotr Findeisen Date: Tue, 20 Jun 2023 10:00:49 +0200 Subject: [PATCH 4/8] Describe test location patterns with enum The `String.contains`-based inclusions/exclusions doesn't allow distinguishing between hypothetical cases like "trailing_slash" and "two_trailing_slashes". This also simplifies generated test locations for schemas. Previously the schema name was formatted twice into the location string. --- .../hive/BaseS3AndGlueMetastoreTest.java | 60 ++++++++++++------- .../hive/TestHiveS3AndGlueMetastoreTest.java | 23 +++---- .../TestIcebergS3AndGlueMetastoreTest.java | 4 +- 3 files changed, 52 insertions(+), 35 deletions(-) diff --git a/plugin/trino-hive/src/test/java/io/trino/plugin/hive/BaseS3AndGlueMetastoreTest.java b/plugin/trino-hive/src/test/java/io/trino/plugin/hive/BaseS3AndGlueMetastoreTest.java index 627a5c972a8e..238125ffd3d6 100644 --- a/plugin/trino-hive/src/test/java/io/trino/plugin/hive/BaseS3AndGlueMetastoreTest.java +++ b/plugin/trino-hive/src/test/java/io/trino/plugin/hive/BaseS3AndGlueMetastoreTest.java @@ -17,7 +17,6 @@ import com.amazonaws.services.s3.AmazonS3ClientBuilder; import com.amazonaws.services.s3.model.ListObjectsV2Request; import com.amazonaws.services.s3.model.S3ObjectSummary; -import com.google.common.collect.ImmutableList; import io.trino.Session; import io.trino.plugin.hive.metastore.HiveMetastore; import io.trino.spi.connector.SchemaNotFoundException; @@ -32,6 +31,7 @@ import java.util.regex.Matcher; import java.util.regex.Pattern; import java.util.stream.Collectors; +import java.util.stream.Stream; import static com.google.common.base.Verify.verify; import static com.google.common.collect.Sets.union; @@ -85,26 +85,14 @@ public void tearDown() @DataProvider public Object[][] locationPatternsDataProvider() { - return cartesianProduct(trueFalse(), locationPatterns().stream().collect(toDataProvider())); - } - - protected List locationPatterns() - { - return ImmutableList.builder() - .add("s3://%s/%s/regular/%s") - .add("s3://%s/%s/trailing_slash/%s/") - .add("s3://%s/%s//double_slash/%s") - .add("s3://%s/%s/a%%percent/%s") - .add("s3://%s/%s/a whitespace/%s") - .add("s3://%s/%s/trailing_whitespace/%s ") - .build(); + return cartesianProduct(trueFalse(), Stream.of(LocationPattern.values()).collect(toDataProvider())); } @Test(dataProvider = "locationPatternsDataProvider") - public void testBasicOperationsWithProvidedTableLocation(boolean partitioned, String locationPattern) + public void testBasicOperationsWithProvidedTableLocation(boolean partitioned, LocationPattern locationPattern) { String tableName = "test_basic_operations_" + randomNameSuffix(); - String location = locationPattern.formatted(bucketName, schemaName, tableName); + String location = locationPattern.locationForTable(bucketName, schemaName, tableName); String partitionQueryPart = (partitioned ? "," + partitionByKeyword + " = ARRAY['col_str']" : ""); assertUpdate("CREATE TABLE " + tableName + "(col_str, col_int)" + @@ -131,10 +119,10 @@ public void testBasicOperationsWithProvidedTableLocation(boolean partitioned, St } @Test(dataProvider = "locationPatternsDataProvider") - public void testBasicOperationsWithProvidedSchemaLocation(boolean partitioned, String locationPattern) + public void testBasicOperationsWithProvidedSchemaLocation(boolean partitioned, LocationPattern locationPattern) { String schemaName = "test_basic_operations_schema_" + randomNameSuffix(); - String schemaLocation = locationPattern.formatted(bucketName, schemaName, schemaName); + String schemaLocation = locationPattern.locationForSchema(bucketName, schemaName); String tableName = "test_basic_operations_table_" + randomNameSuffix(); String qualifiedTableName = schemaName + "." + tableName; String partitionQueryPart = (partitioned ? "WITH (" + partitionByKeyword + " = ARRAY['col_str'])" : ""); @@ -169,10 +157,10 @@ public void testBasicOperationsWithProvidedSchemaLocation(boolean partitioned, S } @Test(dataProvider = "locationPatternsDataProvider") - public void testMergeWithProvidedTableLocation(boolean partitioned, String locationPattern) + public void testMergeWithProvidedTableLocation(boolean partitioned, LocationPattern locationPattern) { String tableName = "test_merge_" + randomNameSuffix(); - String location = locationPattern.formatted(bucketName, schemaName, tableName); + String location = locationPattern.locationForTable(bucketName, schemaName, tableName); String partitionQueryPart = (partitioned ? "," + partitionByKeyword + " = ARRAY['col_str']" : ""); assertUpdate("CREATE TABLE " + tableName + "(col_str, col_int)" + @@ -201,10 +189,10 @@ public void testMergeWithProvidedTableLocation(boolean partitioned, String locat } @Test(dataProvider = "locationPatternsDataProvider") - public void testOptimizeWithProvidedTableLocation(boolean partitioned, String locationPattern) + public void testOptimizeWithProvidedTableLocation(boolean partitioned, LocationPattern locationPattern) { String tableName = "test_optimize_" + randomNameSuffix(); - String location = locationPattern.formatted(bucketName, schemaName, tableName); + String location = locationPattern.locationForTable(bucketName, schemaName, tableName); String partitionQueryPart = (partitioned ? "," + partitionByKeyword + " = ARRAY['value']" : ""); String locationQueryPart = locationKeyword + "= '" + location + "'"; @@ -313,4 +301,32 @@ protected void verifyPathExist(String path) { assertThat(s3Path(s3, path)).exists(); } + + protected enum LocationPattern + { + REGULAR("s3://%s/%s/regular/%s"), + TRAILING_SLASH("s3://%s/%s/trailing_slash/%s/"), + DOUBLE_SLASH("s3://%s/%s//double_slash/%s"), + PERCENT("s3://%s/%s/a%%percent/%s"), + WHITESPACE("s3://%s/%s/a whitespace/%s"), + TRAILING_WHITESPACE("s3://%s/%s/trailing_whitespace/%s "), + /**/; + + private final String locationPattern; + + LocationPattern(String locationPattern) + { + this.locationPattern = requireNonNull(locationPattern, "locationPattern is null"); + } + + public String locationForSchema(String bucketName, String schemaName) + { + return locationPattern.formatted(bucketName, "warehouse", schemaName); + } + + public String locationForTable(String bucketName, String schemaName, String tableName) + { + return locationPattern.formatted(bucketName, schemaName, tableName); + } + } } diff --git a/plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHiveS3AndGlueMetastoreTest.java b/plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHiveS3AndGlueMetastoreTest.java index 34cb9632b93f..eb12f9e0ab30 100644 --- a/plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHiveS3AndGlueMetastoreTest.java +++ b/plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHiveS3AndGlueMetastoreTest.java @@ -26,6 +26,7 @@ import java.util.Optional; import java.util.Set; +import static io.trino.plugin.hive.BaseS3AndGlueMetastoreTest.LocationPattern.DOUBLE_SLASH; import static io.trino.plugin.hive.metastore.glue.GlueHiveMetastore.createTestingGlueHiveMetastore; import static io.trino.spi.security.SelectedRole.Type.ROLE; import static io.trino.testing.TestingNames.randomNameSuffix; @@ -112,16 +113,16 @@ protected void validateFilesAfterOptimize(String location, Set initialFi @Override // Row-level modifications are not supported for Hive tables @Test(dataProvider = "locationPatternsDataProvider") - public void testBasicOperationsWithProvidedTableLocation(boolean partitioned, String locationPattern) + public void testBasicOperationsWithProvidedTableLocation(boolean partitioned, LocationPattern locationPattern) { String tableName = "test_basic_operations_" + randomNameSuffix(); - String location = locationPattern.formatted(bucketName, schemaName, tableName); + String location = locationPattern.locationForTable(bucketName, schemaName, tableName); String partitionQueryPart = (partitioned ? ",partitioned_by = ARRAY['col_int']" : ""); String create = "CREATE TABLE " + tableName + "(col_str, col_int)" + "WITH (external_location = '" + location + "'" + partitionQueryPart + ") " + "AS VALUES ('str1', 1), ('str2', 2), ('str3', 3)"; - if (locationPattern.contains("double_slash")) { + if (locationPattern == DOUBLE_SLASH) { assertQueryFails(create, "\\QUnsupported location that cannot be internally represented: " + location); return; } @@ -142,10 +143,10 @@ public void testBasicOperationsWithProvidedTableLocation(boolean partitioned, St @Override // Row-level modifications are not supported for Hive tables @Test(dataProvider = "locationPatternsDataProvider") - public void testBasicOperationsWithProvidedSchemaLocation(boolean partitioned, String locationPattern) + public void testBasicOperationsWithProvidedSchemaLocation(boolean partitioned, LocationPattern locationPattern) { String schemaName = "test_basic_operations_schema_" + randomNameSuffix(); - String schemaLocation = locationPattern.formatted(bucketName, schemaName, schemaName); + String schemaLocation = locationPattern.locationForSchema(bucketName, schemaName); String tableName = "test_basic_operations_table_" + randomNameSuffix(); String qualifiedTableName = schemaName + "." + tableName; String partitionQueryPart = (partitioned ? " WITH (partitioned_by = ARRAY['col_int'])" : ""); @@ -176,15 +177,15 @@ public void testBasicOperationsWithProvidedSchemaLocation(boolean partitioned, S @Override @Test(dataProvider = "locationPatternsDataProvider") - public void testMergeWithProvidedTableLocation(boolean partitioned, String locationPattern) + public void testMergeWithProvidedTableLocation(boolean partitioned, LocationPattern locationPattern) { // Row-level modifications are not supported for Hive tables } @Override - public void testOptimizeWithProvidedTableLocation(boolean partitioned, String locationPattern) + public void testOptimizeWithProvidedTableLocation(boolean partitioned, LocationPattern locationPattern) { - if (locationPattern.contains("double_slash")) { + if (locationPattern == DOUBLE_SLASH) { assertThatThrownBy(() -> super.testOptimizeWithProvidedTableLocation(partitioned, locationPattern)) .hasMessageStartingWith("Unsupported location that cannot be internally represented: ") .hasStackTraceContaining("SQL: CREATE TABLE test_optimize_"); @@ -194,16 +195,16 @@ public void testOptimizeWithProvidedTableLocation(boolean partitioned, String lo } @Test(dataProvider = "locationPatternsDataProvider") - public void testAnalyzeWithProvidedTableLocation(boolean partitioned, String locationPattern) + public void testAnalyzeWithProvidedTableLocation(boolean partitioned, LocationPattern locationPattern) { String tableName = "test_analyze_" + randomNameSuffix(); - String location = locationPattern.formatted(bucketName, schemaName, tableName); + String location = locationPattern.locationForTable(bucketName, schemaName, tableName); String partitionQueryPart = (partitioned ? ",partitioned_by = ARRAY['col_int']" : ""); String create = "CREATE TABLE " + tableName + "(col_str, col_int)" + "WITH (external_location = '" + location + "'" + partitionQueryPart + ") " + "AS VALUES ('str1', 1), ('str2', 2), ('str3', 3)"; - if (locationPattern.contains("double_slash")) { + if (locationPattern == DOUBLE_SLASH) { assertQueryFails(create, "\\QUnsupported location that cannot be internally represented: " + location); return; } diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/glue/TestIcebergS3AndGlueMetastoreTest.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/glue/TestIcebergS3AndGlueMetastoreTest.java index 0de6bd41a363..a89087c0b2f2 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/glue/TestIcebergS3AndGlueMetastoreTest.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/glue/TestIcebergS3AndGlueMetastoreTest.java @@ -102,10 +102,10 @@ protected Set getAllDataFilesFromTableDirectory(String tableLocation) } @Test(dataProvider = "locationPatternsDataProvider") - public void testAnalyzeWithProvidedTableLocation(boolean partitioned, String locationPattern) + public void testAnalyzeWithProvidedTableLocation(boolean partitioned, LocationPattern locationPattern) { String tableName = "test_analyze_" + randomNameSuffix(); - String location = locationPattern.formatted(bucketName, schemaName, tableName); + String location = locationPattern.locationForTable(bucketName, schemaName, tableName); String partitionQueryPart = (partitioned ? ",partitioning = ARRAY['col_str']" : ""); assertUpdate("CREATE TABLE " + tableName + "(col_str, col_int)" + From 919452e995324275f0850850c480bfd5427f551d Mon Sep 17 00:00:00 2001 From: Piotr Findeisen Date: Tue, 20 Jun 2023 11:11:29 +0200 Subject: [PATCH 5/8] Remove unused and defunct TestTable constructor Remove constructor which doesn't construct the table. Since it was randomizing the name, it couldn't even be used for dropping existing table. --- .../src/main/java/io/trino/testing/sql/TestTable.java | 7 ------- 1 file changed, 7 deletions(-) diff --git a/testing/trino-testing/src/main/java/io/trino/testing/sql/TestTable.java b/testing/trino-testing/src/main/java/io/trino/testing/sql/TestTable.java index 8f80948ab500..4e125cdd4ef9 100644 --- a/testing/trino-testing/src/main/java/io/trino/testing/sql/TestTable.java +++ b/testing/trino-testing/src/main/java/io/trino/testing/sql/TestTable.java @@ -44,13 +44,6 @@ public TestTable(SqlExecutor sqlExecutor, String namePrefix, String tableDefinit createAndInsert(rowsToInsert); } - public TestTable(SqlExecutor sqlExecutor, String namePrefix) - { - this.sqlExecutor = sqlExecutor; - this.name = namePrefix + randomNameSuffix(); - this.tableDefinition = null; - } - public void createAndInsert(List rowsToInsert) { sqlExecutor.execute(format("CREATE TABLE %s %s", name, tableDefinition)); From 8ed4060c22ee70dcde17a975e18be8728b8f3c01 Mon Sep 17 00:00:00 2001 From: Piotr Findeisen Date: Tue, 20 Jun 2023 11:22:07 +0200 Subject: [PATCH 6/8] Ensure test resource cleanup in BaseS3AndGlueMetastoreTest Use try with resources to ensure that test tables are dropped even if an assertion fails. --- .../hive/BaseS3AndGlueMetastoreTest.java | 126 ++++++++------- .../hive/TestHiveS3AndGlueMetastoreTest.java | 146 +++++++++--------- .../TestIcebergS3AndGlueMetastoreTest.java | 53 ++++--- 3 files changed, 168 insertions(+), 157 deletions(-) diff --git a/plugin/trino-hive/src/test/java/io/trino/plugin/hive/BaseS3AndGlueMetastoreTest.java b/plugin/trino-hive/src/test/java/io/trino/plugin/hive/BaseS3AndGlueMetastoreTest.java index 238125ffd3d6..f99e18205552 100644 --- a/plugin/trino-hive/src/test/java/io/trino/plugin/hive/BaseS3AndGlueMetastoreTest.java +++ b/plugin/trino-hive/src/test/java/io/trino/plugin/hive/BaseS3AndGlueMetastoreTest.java @@ -21,6 +21,7 @@ import io.trino.plugin.hive.metastore.HiveMetastore; import io.trino.spi.connector.SchemaNotFoundException; import io.trino.testing.AbstractTestQueryFramework; +import org.intellij.lang.annotations.Language; import org.testng.annotations.AfterClass; import org.testng.annotations.BeforeClass; import org.testng.annotations.DataProvider; @@ -98,23 +99,23 @@ public void testBasicOperationsWithProvidedTableLocation(boolean partitioned, Lo assertUpdate("CREATE TABLE " + tableName + "(col_str, col_int)" + "WITH (location = '" + location + "'" + partitionQueryPart + ") " + "AS VALUES ('str1', 1), ('str2', 2), ('str3', 3)", 3); - assertQuery("SELECT * FROM " + tableName, "VALUES ('str1', 1), ('str2', 2), ('str3', 3)"); - validateTableLocation(tableName, location); + try (UncheckedCloseable ignored = onClose("DROP TABLE " + tableName)) { + assertQuery("SELECT * FROM " + tableName, "VALUES ('str1', 1), ('str2', 2), ('str3', 3)"); + validateTableLocation(tableName, location); - assertUpdate("INSERT INTO " + tableName + " VALUES ('str4', 4)", 1); - assertQuery("SELECT * FROM " + tableName, "VALUES ('str1', 1), ('str2', 2), ('str3', 3), ('str4', 4)"); + assertUpdate("INSERT INTO " + tableName + " VALUES ('str4', 4)", 1); + assertQuery("SELECT * FROM " + tableName, "VALUES ('str1', 1), ('str2', 2), ('str3', 3), ('str4', 4)"); - assertUpdate("UPDATE " + tableName + " SET col_str = 'other' WHERE col_int = 2", 1); - assertQuery("SELECT * FROM " + tableName, "VALUES ('str1', 1), ('other', 2), ('str3', 3), ('str4', 4)"); + assertUpdate("UPDATE " + tableName + " SET col_str = 'other' WHERE col_int = 2", 1); + assertQuery("SELECT * FROM " + tableName, "VALUES ('str1', 1), ('other', 2), ('str3', 3), ('str4', 4)"); - assertUpdate("DELETE FROM " + tableName + " WHERE col_int = 3", 1); - assertQuery("SELECT * FROM " + tableName, "VALUES ('str1', 1), ('other', 2), ('str4', 4)"); + assertUpdate("DELETE FROM " + tableName + " WHERE col_int = 3", 1); + assertQuery("SELECT * FROM " + tableName, "VALUES ('str1', 1), ('other', 2), ('str4', 4)"); - assertThat(getTableFiles(location)).isNotEmpty(); - validateDataFiles(partitioned ? "col_str" : "", tableName, location); - validateMetadataFiles(location); - - assertUpdate("DROP TABLE " + tableName); + assertThat(getTableFiles(location)).isNotEmpty(); + validateDataFiles(partitioned ? "col_str" : "", tableName, location); + validateMetadataFiles(location); + } validateFilesAfterDrop(location); } @@ -127,32 +128,33 @@ public void testBasicOperationsWithProvidedSchemaLocation(boolean partitioned, L String qualifiedTableName = schemaName + "." + tableName; String partitionQueryPart = (partitioned ? "WITH (" + partitionByKeyword + " = ARRAY['col_str'])" : ""); + String actualTableLocation; assertUpdate("CREATE SCHEMA " + schemaName + " WITH (location = '" + schemaLocation + "')"); - assertThat(getSchemaLocation(schemaName)).isEqualTo(schemaLocation); - - assertUpdate("CREATE TABLE " + qualifiedTableName + "(col_int int, col_str varchar)" + partitionQueryPart); - // in case of regular CREATE TABLE, location has generated suffix - String expectedTableLocationPattern = (schemaLocation.endsWith("/") ? schemaLocation : schemaLocation + "/") + tableName + "-[a-z0-9]+"; - String actualTableLocation = getTableLocation(qualifiedTableName); - assertThat(actualTableLocation).matches(expectedTableLocationPattern); - - assertUpdate("INSERT INTO " + qualifiedTableName + " (col_str, col_int) VALUES ('str1', 1), ('str2', 2), ('str3', 3)", 3); - assertQuery("SELECT col_str, col_int FROM " + qualifiedTableName, "VALUES ('str1', 1), ('str2', 2), ('str3', 3)"); - - assertUpdate("UPDATE " + qualifiedTableName + " SET col_str = 'other' WHERE col_int = 2", 1); - assertQuery("SELECT col_str, col_int FROM " + qualifiedTableName, "VALUES ('str1', 1), ('other', 2), ('str3', 3)"); - - assertUpdate("DELETE FROM " + qualifiedTableName + " WHERE col_int = 3", 1); - assertQuery("SELECT col_str, col_int FROM " + qualifiedTableName, "VALUES ('str1', 1), ('other', 2)"); - - assertThat(getTableFiles(actualTableLocation)).isNotEmpty(); - validateDataFiles(partitioned ? "col_str" : "", qualifiedTableName, actualTableLocation); - validateMetadataFiles(actualTableLocation); - - assertUpdate("DROP TABLE " + qualifiedTableName); - assertThat(getTableFiles(actualTableLocation)).isEmpty(); - - assertUpdate("DROP SCHEMA " + schemaName); + try (UncheckedCloseable ignoredDropSchema = onClose("DROP SCHEMA " + schemaName)) { + assertThat(getSchemaLocation(schemaName)).isEqualTo(schemaLocation); + + assertUpdate("CREATE TABLE " + qualifiedTableName + "(col_int int, col_str varchar)" + partitionQueryPart); + try (UncheckedCloseable ignoredDropTable = onClose("DROP TABLE " + qualifiedTableName)) { + // in case of regular CREATE TABLE, location has generated suffix + String expectedTableLocationPattern = (schemaLocation.endsWith("/") ? schemaLocation : schemaLocation + "/") + tableName + "-[a-z0-9]+"; + actualTableLocation = getTableLocation(qualifiedTableName); + assertThat(actualTableLocation).matches(expectedTableLocationPattern); + + assertUpdate("INSERT INTO " + qualifiedTableName + " (col_str, col_int) VALUES ('str1', 1), ('str2', 2), ('str3', 3)", 3); + assertQuery("SELECT col_str, col_int FROM " + qualifiedTableName, "VALUES ('str1', 1), ('str2', 2), ('str3', 3)"); + + assertUpdate("UPDATE " + qualifiedTableName + " SET col_str = 'other' WHERE col_int = 2", 1); + assertQuery("SELECT col_str, col_int FROM " + qualifiedTableName, "VALUES ('str1', 1), ('other', 2), ('str3', 3)"); + + assertUpdate("DELETE FROM " + qualifiedTableName + " WHERE col_int = 3", 1); + assertQuery("SELECT col_str, col_int FROM " + qualifiedTableName, "VALUES ('str1', 1), ('other', 2)"); + + assertThat(getTableFiles(actualTableLocation)).isNotEmpty(); + validateDataFiles(partitioned ? "col_str" : "", qualifiedTableName, actualTableLocation); + validateMetadataFiles(actualTableLocation); + } + assertThat(getTableFiles(actualTableLocation)).isEmpty(); + } assertThat(getTableFiles(actualTableLocation)).isEmpty(); } @@ -166,25 +168,25 @@ public void testMergeWithProvidedTableLocation(boolean partitioned, LocationPatt assertUpdate("CREATE TABLE " + tableName + "(col_str, col_int)" + "WITH (location = '" + location + "'" + partitionQueryPart + ") " + "AS VALUES ('str1', 1), ('str2', 2), ('str3', 3)", 3); - assertQuery("SELECT * FROM " + tableName, "VALUES ('str1', 1), ('str2', 2), ('str3', 3)"); - - assertUpdate("MERGE INTO " + tableName + " USING (VALUES 1) t(x) ON false" + - " WHEN NOT MATCHED THEN INSERT VALUES ('str4', 4)", 1); - assertQuery("SELECT * FROM " + tableName, "VALUES ('str1', 1), ('str2', 2), ('str3', 3), ('str4', 4)"); + try (UncheckedCloseable ignored = onClose("DROP TABLE " + tableName)) { + assertQuery("SELECT * FROM " + tableName, "VALUES ('str1', 1), ('str2', 2), ('str3', 3)"); - assertUpdate("MERGE INTO " + tableName + " USING (VALUES 2) t(x) ON col_int = x" + - " WHEN MATCHED THEN UPDATE SET col_str = 'other'", 1); - assertQuery("SELECT * FROM " + tableName, "VALUES ('str1', 1), ('other', 2), ('str3', 3), ('str4', 4)"); + assertUpdate("MERGE INTO " + tableName + " USING (VALUES 1) t(x) ON false" + + " WHEN NOT MATCHED THEN INSERT VALUES ('str4', 4)", 1); + assertQuery("SELECT * FROM " + tableName, "VALUES ('str1', 1), ('str2', 2), ('str3', 3), ('str4', 4)"); - assertUpdate("MERGE INTO " + tableName + " USING (VALUES 3) t(x) ON col_int = x" + - " WHEN MATCHED THEN DELETE", 1); - assertQuery("SELECT * FROM " + tableName, "VALUES ('str1', 1), ('other', 2), ('str4', 4)"); + assertUpdate("MERGE INTO " + tableName + " USING (VALUES 2) t(x) ON col_int = x" + + " WHEN MATCHED THEN UPDATE SET col_str = 'other'", 1); + assertQuery("SELECT * FROM " + tableName, "VALUES ('str1', 1), ('other', 2), ('str3', 3), ('str4', 4)"); - assertThat(getTableFiles(location)).isNotEmpty(); - validateDataFiles(partitioned ? "col_str" : "", tableName, location); - validateMetadataFiles(location); + assertUpdate("MERGE INTO " + tableName + " USING (VALUES 3) t(x) ON col_int = x" + + " WHEN MATCHED THEN DELETE", 1); + assertQuery("SELECT * FROM " + tableName, "VALUES ('str1', 1), ('other', 2), ('str4', 4)"); - assertUpdate("DROP TABLE " + tableName); + assertThat(getTableFiles(location)).isNotEmpty(); + validateDataFiles(partitioned ? "col_str" : "", tableName, location); + validateMetadataFiles(location); + } validateFilesAfterDrop(location); } @@ -198,7 +200,7 @@ public void testOptimizeWithProvidedTableLocation(boolean partitioned, LocationP assertUpdate("CREATE TABLE " + tableName + " (key integer, value varchar) " + "WITH (" + locationQueryPart + partitionQueryPart + ")"); - try { + try (UncheckedCloseable ignored = onClose("DROP TABLE " + tableName)) { // create multiple data files, INSERT with multiple values would create only one file (if not partitioned) assertUpdate("INSERT INTO " + tableName + " VALUES (1, 'one')", 1); assertUpdate("INSERT INTO " + tableName + " VALUES (2, 'a//double_slash')", 1); @@ -218,9 +220,6 @@ public void testOptimizeWithProvidedTableLocation(boolean partitioned, LocationP Set updatedFiles = getActiveFiles(tableName); validateFilesAfterOptimize(getTableLocation(tableName), initialFiles, updatedFiles); } - finally { - assertUpdate("DROP TABLE " + tableName); - } } protected Session sessionForOptimize() @@ -292,6 +291,12 @@ protected List getTableFiles(String location) .toList(); } + protected UncheckedCloseable onClose(@Language("SQL") String sql) + { + requireNonNull(sql, "sql is null"); + return () -> assertUpdate(sql); + } + protected String schemaPath() { return "s3://%s/%s".formatted(bucketName, schemaName); @@ -329,4 +334,11 @@ public String locationForTable(String bucketName, String schemaName, String tabl return locationPattern.formatted(bucketName, schemaName, tableName); } } + + protected interface UncheckedCloseable + extends AutoCloseable + { + @Override + void close(); + } } diff --git a/plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHiveS3AndGlueMetastoreTest.java b/plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHiveS3AndGlueMetastoreTest.java index eb12f9e0ab30..2b795e340f47 100644 --- a/plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHiveS3AndGlueMetastoreTest.java +++ b/plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHiveS3AndGlueMetastoreTest.java @@ -127,18 +127,18 @@ public void testBasicOperationsWithProvidedTableLocation(boolean partitioned, Lo return; } assertUpdate(create, 3); - assertQuery("SELECT * FROM " + tableName, "VALUES ('str1', 1), ('str2', 2), ('str3', 3)"); + try (UncheckedCloseable ignored = onClose("DROP TABLE " + tableName)) { + assertQuery("SELECT * FROM " + tableName, "VALUES ('str1', 1), ('str2', 2), ('str3', 3)"); - String actualTableLocation = getTableLocation(tableName); - assertThat(actualTableLocation).isEqualTo(location); + String actualTableLocation = getTableLocation(tableName); + assertThat(actualTableLocation).isEqualTo(location); - assertUpdate("INSERT INTO " + tableName + " VALUES ('str4', 4)", 1); - assertQuery("SELECT * FROM " + tableName, "VALUES ('str1', 1), ('str2', 2), ('str3', 3), ('str4', 4)"); + assertUpdate("INSERT INTO " + tableName + " VALUES ('str4', 4)", 1); + assertQuery("SELECT * FROM " + tableName, "VALUES ('str1', 1), ('str2', 2), ('str3', 3), ('str4', 4)"); - assertThat(getTableFiles(actualTableLocation)).isNotEmpty(); - validateDataFiles(partitioned ? "col_int" : "", tableName, actualTableLocation); - - assertUpdate("DROP TABLE " + tableName); + assertThat(getTableFiles(actualTableLocation)).isNotEmpty(); + validateDataFiles(partitioned ? "col_int" : "", tableName, actualTableLocation); + } } @Override // Row-level modifications are not supported for Hive tables @@ -151,27 +151,28 @@ public void testBasicOperationsWithProvidedSchemaLocation(boolean partitioned, L String qualifiedTableName = schemaName + "." + tableName; String partitionQueryPart = (partitioned ? " WITH (partitioned_by = ARRAY['col_int'])" : ""); + String actualTableLocation; assertUpdate("CREATE SCHEMA " + schemaName + " WITH (location = '" + schemaLocation + "')"); - assertThat(getSchemaLocation(schemaName)).isEqualTo(schemaLocation); - - assertUpdate("CREATE TABLE " + qualifiedTableName + "(col_str varchar, col_int int)" + partitionQueryPart); - String expectedTableLocation = ((schemaLocation.endsWith("/") ? schemaLocation : schemaLocation + "/") + tableName) - // Hive normalizes double slash - .replaceAll("(? assertUpdate("CREATE TABLE " + qualifiedTableName + "(col_str, col_int) AS VALUES ('str1', 1)")) - .hasMessageContaining("Fragment is not allowed in a file system location"); + assertThatThrownBy(() -> assertUpdate("CREATE TABLE " + qualifiedTableName + "(col_str, col_int) AS VALUES ('str1', 1)")) + .hasMessageContaining("Fragment is not allowed in a file system location"); - assertThatThrownBy(() -> assertUpdate("CREATE TABLE " + qualifiedTableName + "(col_str varchar, col_int integer)")) - .hasMessageContaining("Fragment is not allowed in a file system location"); - - assertUpdate("DROP SCHEMA " + schemaName); + assertThatThrownBy(() -> assertUpdate("CREATE TABLE " + qualifiedTableName + "(col_str varchar, col_int integer)")) + .hasMessageContaining("Fragment is not allowed in a file system location"); + } } @Test @@ -302,8 +302,8 @@ public void testSchemaNameEscape() String tableName = "test_table_schema_escaped_" + randomNameSuffix(); assertUpdate("CREATE SCHEMA \"%2$s\" WITH (location = 's3://%1$s/%2$s')".formatted(bucketName, schemaName)); - assertQueryFails("CREATE TABLE \"" + schemaName + "\"." + tableName + " (col) AS VALUES 1", "Failed checking path: .*"); - - assertUpdate("DROP SCHEMA \"" + schemaName + "\""); + try (UncheckedCloseable ignored = onClose("DROP SCHEMA \"" + schemaName + "\"")) { + assertQueryFails("CREATE TABLE \"" + schemaName + "\"." + tableName + " (col) AS VALUES 1", "Failed checking path: .*"); + } } } diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/glue/TestIcebergS3AndGlueMetastoreTest.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/glue/TestIcebergS3AndGlueMetastoreTest.java index a89087c0b2f2..541e4aa032bc 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/glue/TestIcebergS3AndGlueMetastoreTest.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/glue/TestIcebergS3AndGlueMetastoreTest.java @@ -111,26 +111,25 @@ public void testAnalyzeWithProvidedTableLocation(boolean partitioned, LocationPa assertUpdate("CREATE TABLE " + tableName + "(col_str, col_int)" + "WITH (location = '" + location + "'" + partitionQueryPart + ") " + "AS VALUES ('str1', 1), ('str2', 2), ('str3', 3)", 3); - - assertUpdate("INSERT INTO " + tableName + " VALUES ('str4', 4)", 1); - assertQuery("SELECT * FROM " + tableName, "VALUES ('str1', 1), ('str2', 2), ('str3', 3), ('str4', 4)"); - - String expectedStatistics = """ - VALUES - ('col_str', null, 4.0, 0.0, null, null, null), - ('col_int', null, 4.0, 0.0, null, 1, 4), - (null, null, null, null, 4.0, null, null)"""; - - // Check extended statistics collection on write - assertQuery("SHOW STATS FOR " + tableName, expectedStatistics); - - // drop stats - assertUpdate("ALTER TABLE " + tableName + " EXECUTE DROP_EXTENDED_STATS"); - // Check extended statistics collection explicitly - assertUpdate("ANALYZE " + tableName); - assertQuery("SHOW STATS FOR " + tableName, expectedStatistics); - - assertUpdate("DROP TABLE " + tableName); + try (UncheckedCloseable ignored = onClose("DROP TABLE " + tableName)) { + assertUpdate("INSERT INTO " + tableName + " VALUES ('str4', 4)", 1); + assertQuery("SELECT * FROM " + tableName, "VALUES ('str1', 1), ('str2', 2), ('str3', 3), ('str4', 4)"); + + String expectedStatistics = """ + VALUES + ('col_str', null, 4.0, 0.0, null, null, null), + ('col_int', null, 4.0, 0.0, null, 1, 4), + (null, null, null, null, 4.0, null, null)"""; + + // Check extended statistics collection on write + assertQuery("SHOW STATS FOR " + tableName, expectedStatistics); + + // drop stats + assertUpdate("ALTER TABLE " + tableName + " EXECUTE DROP_EXTENDED_STATS"); + // Check extended statistics collection explicitly + assertUpdate("ANALYZE " + tableName); + assertQuery("SHOW STATS FOR " + tableName, expectedStatistics); + } } @Test @@ -164,14 +163,14 @@ public void testCreateSchemaWithIncorrectLocation() String qualifiedTableName = schemaName + "." + tableName; assertUpdate("CREATE SCHEMA " + schemaName + " WITH (location = '" + schemaLocation + "')"); - assertThat(getSchemaLocation(schemaName)).isEqualTo(schemaLocation); + try (UncheckedCloseable ignored = onClose("DROP SCHEMA " + schemaName)) { + assertThat(getSchemaLocation(schemaName)).isEqualTo(schemaLocation); - assertThatThrownBy(() -> assertUpdate("CREATE TABLE " + qualifiedTableName + "(col_str varchar, col_int int)")) - .hasMessageContaining("location contains a fragment"); + assertThatThrownBy(() -> assertUpdate("CREATE TABLE " + qualifiedTableName + "(col_str varchar, col_int int)")) + .hasMessageContaining("location contains a fragment"); - assertThatThrownBy(() -> assertUpdate("CREATE TABLE " + qualifiedTableName + " AS SELECT * FROM tpch.tiny.nation")) - .hasMessageContaining("location contains a fragment"); - - assertUpdate("DROP SCHEMA " + schemaName); + assertThatThrownBy(() -> assertUpdate("CREATE TABLE " + qualifiedTableName + " AS SELECT * FROM tpch.tiny.nation")) + .hasMessageContaining("location contains a fragment"); + } } } From c029554678f3f6485ea96c20a020a124863e1197 Mon Sep 17 00:00:00 2001 From: Piotr Findeisen Date: Tue, 20 Jun 2023 09:12:33 +0200 Subject: [PATCH 7/8] Add more double slashes related test cases --- .../hive/BaseS3AndGlueMetastoreTest.java | 63 +++++++++++++++---- .../hive/TestHiveS3AndGlueMetastoreTest.java | 12 ++-- .../TestIcebergS3AndGlueMetastoreTest.java | 10 +-- 3 files changed, 63 insertions(+), 22 deletions(-) diff --git a/plugin/trino-hive/src/test/java/io/trino/plugin/hive/BaseS3AndGlueMetastoreTest.java b/plugin/trino-hive/src/test/java/io/trino/plugin/hive/BaseS3AndGlueMetastoreTest.java index f99e18205552..0e1a2fb3a31f 100644 --- a/plugin/trino-hive/src/test/java/io/trino/plugin/hive/BaseS3AndGlueMetastoreTest.java +++ b/plugin/trino-hive/src/test/java/io/trino/plugin/hive/BaseS3AndGlueMetastoreTest.java @@ -36,6 +36,7 @@ import static com.google.common.base.Verify.verify; import static com.google.common.collect.Sets.union; +import static io.trino.plugin.hive.BaseS3AndGlueMetastoreTest.LocationPattern.TWO_TRAILING_SLASHES; import static io.trino.plugin.hive.S3Assert.s3Path; import static io.trino.testing.DataProviders.cartesianProduct; import static io.trino.testing.DataProviders.toDataProvider; @@ -44,6 +45,7 @@ import static java.lang.String.format; import static java.util.Objects.requireNonNull; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; public abstract class BaseS3AndGlueMetastoreTest extends AbstractTestQueryFramework @@ -96,13 +98,20 @@ public void testBasicOperationsWithProvidedTableLocation(boolean partitioned, Lo String location = locationPattern.locationForTable(bucketName, schemaName, tableName); String partitionQueryPart = (partitioned ? "," + partitionByKeyword + " = ARRAY['col_str']" : ""); + String actualTableLocation; assertUpdate("CREATE TABLE " + tableName + "(col_str, col_int)" + "WITH (location = '" + location + "'" + partitionQueryPart + ") " + "AS VALUES ('str1', 1), ('str2', 2), ('str3', 3)", 3); try (UncheckedCloseable ignored = onClose("DROP TABLE " + tableName)) { assertQuery("SELECT * FROM " + tableName, "VALUES ('str1', 1), ('str2', 2), ('str3', 3)"); - validateTableLocation(tableName, location); + actualTableLocation = validateTableLocation(tableName, location); + if (locationPattern == TWO_TRAILING_SLASHES && getClass().getName().contains(".deltalake.")) { + // TODO (https://github.com/trinodb/trino/issues/17966): writes fail when Delta table is declared with location ending with two slashes + assertThatThrownBy(() -> query("INSERT INTO " + tableName + " VALUES ('str4', 4)")) + .hasMessageStartingWith("Location does not have parent: "); + return; + } assertUpdate("INSERT INTO " + tableName + " VALUES ('str4', 4)", 1); assertQuery("SELECT * FROM " + tableName, "VALUES ('str1', 1), ('str2', 2), ('str3', 3), ('str4', 4)"); @@ -112,11 +121,11 @@ public void testBasicOperationsWithProvidedTableLocation(boolean partitioned, Lo assertUpdate("DELETE FROM " + tableName + " WHERE col_int = 3", 1); assertQuery("SELECT * FROM " + tableName, "VALUES ('str1', 1), ('other', 2), ('str4', 4)"); - assertThat(getTableFiles(location)).isNotEmpty(); - validateDataFiles(partitioned ? "col_str" : "", tableName, location); - validateMetadataFiles(location); + assertThat(getTableFiles(actualTableLocation)).isNotEmpty(); + validateDataFiles(partitioned ? "col_str" : "", tableName, actualTableLocation); + validateMetadataFiles(actualTableLocation); } - validateFilesAfterDrop(location); + validateFilesAfterDrop(actualTableLocation); } @Test(dataProvider = "locationPatternsDataProvider") @@ -140,6 +149,12 @@ public void testBasicOperationsWithProvidedSchemaLocation(boolean partitioned, L actualTableLocation = getTableLocation(qualifiedTableName); assertThat(actualTableLocation).matches(expectedTableLocationPattern); + if (locationPattern == TWO_TRAILING_SLASHES && getClass().getName().contains(".deltalake.")) { + // TODO (https://github.com/trinodb/trino/issues/17966): writes fail when Delta table is declared within schema with location ending with two slashes + assertThatThrownBy(() -> query("INSERT INTO " + qualifiedTableName + " (col_str, col_int) VALUES ('str1', 1), ('str2', 2), ('str3', 3)")) + .hasMessageStartingWith("Location does not have parent: "); + return; + } assertUpdate("INSERT INTO " + qualifiedTableName + " (col_str, col_int) VALUES ('str1', 1), ('str2', 2), ('str3', 3)", 3); assertQuery("SELECT col_str, col_int FROM " + qualifiedTableName, "VALUES ('str1', 1), ('str2', 2), ('str3', 3)"); @@ -165,12 +180,21 @@ public void testMergeWithProvidedTableLocation(boolean partitioned, LocationPatt String location = locationPattern.locationForTable(bucketName, schemaName, tableName); String partitionQueryPart = (partitioned ? "," + partitionByKeyword + " = ARRAY['col_str']" : ""); + String actualTableLocation; assertUpdate("CREATE TABLE " + tableName + "(col_str, col_int)" + "WITH (location = '" + location + "'" + partitionQueryPart + ") " + "AS VALUES ('str1', 1), ('str2', 2), ('str3', 3)", 3); try (UncheckedCloseable ignored = onClose("DROP TABLE " + tableName)) { + actualTableLocation = validateTableLocation(tableName, location); assertQuery("SELECT * FROM " + tableName, "VALUES ('str1', 1), ('str2', 2), ('str3', 3)"); + if (locationPattern == TWO_TRAILING_SLASHES && getClass().getName().contains(".deltalake.")) { + // TODO (https://github.com/trinodb/trino/issues/17966): writes fail when Delta table is declared with location ending with two slashes + assertThatThrownBy(() -> query("MERGE INTO " + tableName + " USING (VALUES 1) t(x) ON false" + + " WHEN NOT MATCHED THEN INSERT VALUES ('str4', 4)")) + .hasMessageStartingWith("Location does not have parent: "); + return; + } assertUpdate("MERGE INTO " + tableName + " USING (VALUES 1) t(x) ON false" + " WHEN NOT MATCHED THEN INSERT VALUES ('str4', 4)", 1); assertQuery("SELECT * FROM " + tableName, "VALUES ('str1', 1), ('str2', 2), ('str3', 3), ('str4', 4)"); @@ -183,11 +207,11 @@ public void testMergeWithProvidedTableLocation(boolean partitioned, LocationPatt " WHEN MATCHED THEN DELETE", 1); assertQuery("SELECT * FROM " + tableName, "VALUES ('str1', 1), ('other', 2), ('str4', 4)"); - assertThat(getTableFiles(location)).isNotEmpty(); - validateDataFiles(partitioned ? "col_str" : "", tableName, location); - validateMetadataFiles(location); + assertThat(getTableFiles(actualTableLocation)).isNotEmpty(); + validateDataFiles(partitioned ? "col_str" : "", tableName, actualTableLocation); + validateMetadataFiles(actualTableLocation); } - validateFilesAfterDrop(location); + validateFilesAfterDrop(actualTableLocation); } @Test(dataProvider = "locationPatternsDataProvider") @@ -202,20 +226,29 @@ public void testOptimizeWithProvidedTableLocation(boolean partitioned, LocationP "WITH (" + locationQueryPart + partitionQueryPart + ")"); try (UncheckedCloseable ignored = onClose("DROP TABLE " + tableName)) { // create multiple data files, INSERT with multiple values would create only one file (if not partitioned) + if (locationPattern == TWO_TRAILING_SLASHES && getClass().getName().contains(".deltalake.")) { + // TODO (https://github.com/trinodb/trino/issues/17966): writes fail when Delta table is declared with location ending with two slashes + assertThatThrownBy(() -> query("INSERT INTO " + tableName + " VALUES (1, 'one')")) + .hasMessageStartingWith("Location does not have parent: "); + return; + } assertUpdate("INSERT INTO " + tableName + " VALUES (1, 'one')", 1); assertUpdate("INSERT INTO " + tableName + " VALUES (2, 'a//double_slash')", 1); assertUpdate("INSERT INTO " + tableName + " VALUES (3, 'a%percent')", 1); assertUpdate("INSERT INTO " + tableName + " VALUES (4, 'a//double_slash')", 1); + assertUpdate("INSERT INTO " + tableName + " VALUES (5, 'a///triple_slash')", 1); + assertUpdate("INSERT INTO " + tableName + " VALUES (6, 'trailing_slash/')", 1); + assertUpdate("INSERT INTO " + tableName + " VALUES (7, 'two_trailing_slashes//')", 1); assertUpdate("INSERT INTO " + tableName + " VALUES (11, 'one')", 1); Set initialFiles = getActiveFiles(tableName); - assertThat(initialFiles).hasSize(5); + assertThat(initialFiles).hasSize(8); Session session = sessionForOptimize(); computeActual(session, "ALTER TABLE " + tableName + " EXECUTE OPTIMIZE"); assertThat(query("SELECT sum(key), listagg(value, ' ') WITHIN GROUP (ORDER BY value) FROM " + tableName)) - .matches("VALUES (BIGINT '21', VARCHAR 'a%percent a//double_slash a//double_slash one one')"); + .matches("VALUES (BIGINT '39', VARCHAR 'a%percent a///triple_slash a//double_slash a//double_slash one one trailing_slash/ two_trailing_slashes//')"); Set updatedFiles = getActiveFiles(tableName); validateFilesAfterOptimize(getTableLocation(tableName), initialFiles, updatedFiles); @@ -237,9 +270,11 @@ protected void validateFilesAfterOptimize(String location, Set initialFi protected abstract void validateMetadataFiles(String location); - protected void validateTableLocation(String tableName, String expectedLocation) + protected String validateTableLocation(String tableName, String expectedLocation) { - assertThat(getTableLocation(tableName)).isEqualTo(expectedLocation); + String actualTableLocation = getTableLocation(tableName); + assertThat(actualTableLocation).isEqualTo(expectedLocation); + return actualTableLocation; } protected void validateFilesAfterDrop(String location) @@ -311,7 +346,9 @@ protected enum LocationPattern { REGULAR("s3://%s/%s/regular/%s"), TRAILING_SLASH("s3://%s/%s/trailing_slash/%s/"), + TWO_TRAILING_SLASHES("s3://%s/%s/two_trailing_slashes/%s//"), DOUBLE_SLASH("s3://%s/%s//double_slash/%s"), + TRIPLE_SLASH("s3://%s/%s///triple_slash/%s"), PERCENT("s3://%s/%s/a%%percent/%s"), WHITESPACE("s3://%s/%s/a whitespace/%s"), TRAILING_WHITESPACE("s3://%s/%s/trailing_whitespace/%s "), diff --git a/plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHiveS3AndGlueMetastoreTest.java b/plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHiveS3AndGlueMetastoreTest.java index 2b795e340f47..5f22419b9204 100644 --- a/plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHiveS3AndGlueMetastoreTest.java +++ b/plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHiveS3AndGlueMetastoreTest.java @@ -27,6 +27,8 @@ import java.util.Set; import static io.trino.plugin.hive.BaseS3AndGlueMetastoreTest.LocationPattern.DOUBLE_SLASH; +import static io.trino.plugin.hive.BaseS3AndGlueMetastoreTest.LocationPattern.TRIPLE_SLASH; +import static io.trino.plugin.hive.BaseS3AndGlueMetastoreTest.LocationPattern.TWO_TRAILING_SLASHES; import static io.trino.plugin.hive.metastore.glue.GlueHiveMetastore.createTestingGlueHiveMetastore; import static io.trino.spi.security.SelectedRole.Type.ROLE; import static io.trino.testing.TestingNames.randomNameSuffix; @@ -122,7 +124,7 @@ public void testBasicOperationsWithProvidedTableLocation(boolean partitioned, Lo String create = "CREATE TABLE " + tableName + "(col_str, col_int)" + "WITH (external_location = '" + location + "'" + partitionQueryPart + ") " + "AS VALUES ('str1', 1), ('str2', 2), ('str3', 3)"; - if (locationPattern == DOUBLE_SLASH) { + if (locationPattern == DOUBLE_SLASH || locationPattern == TRIPLE_SLASH || locationPattern == TWO_TRAILING_SLASHES) { assertQueryFails(create, "\\QUnsupported location that cannot be internally represented: " + location); return; } @@ -159,8 +161,8 @@ public void testBasicOperationsWithProvidedSchemaLocation(boolean partitioned, L assertUpdate("CREATE TABLE " + qualifiedTableName + "(col_str varchar, col_int int)" + partitionQueryPart); try (UncheckedCloseable ignoredDropTable = onClose("DROP TABLE " + qualifiedTableName)) { String expectedTableLocation = ((schemaLocation.endsWith("/") ? schemaLocation : schemaLocation + "/") + tableName) - // Hive normalizes double slash - .replaceAll("(? super.testOptimizeWithProvidedTableLocation(partitioned, locationPattern)) .hasMessageStartingWith("Unsupported location that cannot be internally represented: ") .hasStackTraceContaining("SQL: CREATE TABLE test_optimize_"); @@ -205,7 +207,7 @@ public void testAnalyzeWithProvidedTableLocation(boolean partitioned, LocationPa String create = "CREATE TABLE " + tableName + "(col_str, col_int)" + "WITH (external_location = '" + location + "'" + partitionQueryPart + ") " + "AS VALUES ('str1', 1), ('str2', 2), ('str3', 3)"; - if (locationPattern == DOUBLE_SLASH) { + if (locationPattern == DOUBLE_SLASH || locationPattern == TRIPLE_SLASH || locationPattern == TWO_TRAILING_SLASHES) { assertQueryFails(create, "\\QUnsupported location that cannot be internally represented: " + location); return; } diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/glue/TestIcebergS3AndGlueMetastoreTest.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/glue/TestIcebergS3AndGlueMetastoreTest.java index 541e4aa032bc..df087ed452c0 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/glue/TestIcebergS3AndGlueMetastoreTest.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/glue/TestIcebergS3AndGlueMetastoreTest.java @@ -77,13 +77,15 @@ protected void validateMetadataFiles(String location) } @Override - protected void validateTableLocation(String tableName, String expectedLocation) + protected String validateTableLocation(String tableName, String expectedLocation) { - // Iceberg removes trailing slash from location, and it's expected. + // Iceberg removes trailing slashes from location, and it's expected. if (expectedLocation.endsWith("/")) { - expectedLocation = expectedLocation.replaceFirst("/$", ""); + expectedLocation = expectedLocation.replaceFirst("/+$", ""); } - assertThat(getTableLocation(tableName)).isEqualTo(expectedLocation); + String actualTableLocation = getTableLocation(tableName); + assertThat(actualTableLocation).isEqualTo(expectedLocation); + return actualTableLocation; } private Set getAllMetadataDataFilesFromTableDirectory(String tableLocation) From b6c9cf0bb953a62a47d41edece2fa9bd2c493791 Mon Sep 17 00:00:00 2001 From: Piotr Findeisen Date: Tue, 20 Jun 2023 09:12:33 +0200 Subject: [PATCH 8/8] Fix Delta INSERT when schema OR table location ends with two slashes --- .../plugin/deltalake/DeltaLakeMetadata.java | 3 +- .../hive/BaseS3AndGlueMetastoreTest.java | 38 +++++++------------ 2 files changed, 14 insertions(+), 27 deletions(-) diff --git a/plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeMetadata.java b/plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeMetadata.java index 691c2ae0426c..2b13a018dc94 100644 --- a/plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeMetadata.java +++ b/plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeMetadata.java @@ -168,7 +168,6 @@ import static com.google.common.collect.Sets.difference; import static com.google.common.primitives.Ints.max; import static io.trino.filesystem.Locations.appendPath; -import static io.trino.filesystem.Locations.getParent; import static io.trino.plugin.deltalake.DataFileInfo.DataFileType.DATA; import static io.trino.plugin.deltalake.DeltaLakeAnalyzeProperties.AnalyzeMode.FULL_REFRESH; import static io.trino.plugin.deltalake.DeltaLakeAnalyzeProperties.AnalyzeMode.INCREMENTAL; @@ -1903,7 +1902,7 @@ private void checkWriteAllowed(ConnectorSession session, DeltaLakeTableHandle ta private boolean allowWrite(ConnectorSession session, DeltaLakeTableHandle tableHandle) { try { - String tableMetadataDirectory = appendPath(getParent(tableHandle.getLocation()), tableHandle.getTableName()); + String tableMetadataDirectory = getTransactionLogDir(tableHandle.getLocation()); boolean requiresOptIn = transactionLogWriterFactory.newWriter(session, tableMetadataDirectory).isUnsafe(); return !requiresOptIn || unsafeWritesEnabled; } diff --git a/plugin/trino-hive/src/test/java/io/trino/plugin/hive/BaseS3AndGlueMetastoreTest.java b/plugin/trino-hive/src/test/java/io/trino/plugin/hive/BaseS3AndGlueMetastoreTest.java index 0e1a2fb3a31f..49d6456d1a47 100644 --- a/plugin/trino-hive/src/test/java/io/trino/plugin/hive/BaseS3AndGlueMetastoreTest.java +++ b/plugin/trino-hive/src/test/java/io/trino/plugin/hive/BaseS3AndGlueMetastoreTest.java @@ -106,15 +106,15 @@ public void testBasicOperationsWithProvidedTableLocation(boolean partitioned, Lo assertQuery("SELECT * FROM " + tableName, "VALUES ('str1', 1), ('str2', 2), ('str3', 3)"); actualTableLocation = validateTableLocation(tableName, location); - if (locationPattern == TWO_TRAILING_SLASHES && getClass().getName().contains(".deltalake.")) { - // TODO (https://github.com/trinodb/trino/issues/17966): writes fail when Delta table is declared with location ending with two slashes - assertThatThrownBy(() -> query("INSERT INTO " + tableName + " VALUES ('str4', 4)")) - .hasMessageStartingWith("Location does not have parent: "); - return; - } assertUpdate("INSERT INTO " + tableName + " VALUES ('str4', 4)", 1); assertQuery("SELECT * FROM " + tableName, "VALUES ('str1', 1), ('str2', 2), ('str3', 3), ('str4', 4)"); + if (locationPattern == TWO_TRAILING_SLASHES && !partitioned && getClass().getName().contains(".deltalake.")) { + // TODO (https://github.com/trinodb/trino/issues/17966): updates fail when Delta table is declared with location ending with two slashes + assertThatThrownBy(() -> query("UPDATE " + tableName + " SET col_str = 'other' WHERE col_int = 2")) + .hasMessageMatching("path \\[(s3://.*)/([-a-zA-Z0-9_]+)] must be a subdirectory of basePath \\[(\\1)//]"); + return; + } assertUpdate("UPDATE " + tableName + " SET col_str = 'other' WHERE col_int = 2", 1); assertQuery("SELECT * FROM " + tableName, "VALUES ('str1', 1), ('other', 2), ('str3', 3), ('str4', 4)"); @@ -149,12 +149,6 @@ public void testBasicOperationsWithProvidedSchemaLocation(boolean partitioned, L actualTableLocation = getTableLocation(qualifiedTableName); assertThat(actualTableLocation).matches(expectedTableLocationPattern); - if (locationPattern == TWO_TRAILING_SLASHES && getClass().getName().contains(".deltalake.")) { - // TODO (https://github.com/trinodb/trino/issues/17966): writes fail when Delta table is declared within schema with location ending with two slashes - assertThatThrownBy(() -> query("INSERT INTO " + qualifiedTableName + " (col_str, col_int) VALUES ('str1', 1), ('str2', 2), ('str3', 3)")) - .hasMessageStartingWith("Location does not have parent: "); - return; - } assertUpdate("INSERT INTO " + qualifiedTableName + " (col_str, col_int) VALUES ('str1', 1), ('str2', 2), ('str3', 3)", 3); assertQuery("SELECT col_str, col_int FROM " + qualifiedTableName, "VALUES ('str1', 1), ('str2', 2), ('str3', 3)"); @@ -188,17 +182,17 @@ public void testMergeWithProvidedTableLocation(boolean partitioned, LocationPatt actualTableLocation = validateTableLocation(tableName, location); assertQuery("SELECT * FROM " + tableName, "VALUES ('str1', 1), ('str2', 2), ('str3', 3)"); - if (locationPattern == TWO_TRAILING_SLASHES && getClass().getName().contains(".deltalake.")) { - // TODO (https://github.com/trinodb/trino/issues/17966): writes fail when Delta table is declared with location ending with two slashes - assertThatThrownBy(() -> query("MERGE INTO " + tableName + " USING (VALUES 1) t(x) ON false" + - " WHEN NOT MATCHED THEN INSERT VALUES ('str4', 4)")) - .hasMessageStartingWith("Location does not have parent: "); - return; - } assertUpdate("MERGE INTO " + tableName + " USING (VALUES 1) t(x) ON false" + " WHEN NOT MATCHED THEN INSERT VALUES ('str4', 4)", 1); assertQuery("SELECT * FROM " + tableName, "VALUES ('str1', 1), ('str2', 2), ('str3', 3), ('str4', 4)"); + if (locationPattern == TWO_TRAILING_SLASHES && !partitioned && getClass().getName().contains(".deltalake.")) { + // TODO (https://github.com/trinodb/trino/issues/17966): merge fails when Delta table is declared with location ending with two slashes + assertThatThrownBy(() -> query("MERGE INTO " + tableName + " USING (VALUES 2) t(x) ON col_int = x" + + " WHEN MATCHED THEN UPDATE SET col_str = 'other'")) + .hasMessageMatching("path \\[(s3://.*)/([-a-zA-Z0-9_]+)] must be a subdirectory of basePath \\[(\\1)//]"); + return; + } assertUpdate("MERGE INTO " + tableName + " USING (VALUES 2) t(x) ON col_int = x" + " WHEN MATCHED THEN UPDATE SET col_str = 'other'", 1); assertQuery("SELECT * FROM " + tableName, "VALUES ('str1', 1), ('other', 2), ('str3', 3), ('str4', 4)"); @@ -226,12 +220,6 @@ public void testOptimizeWithProvidedTableLocation(boolean partitioned, LocationP "WITH (" + locationQueryPart + partitionQueryPart + ")"); try (UncheckedCloseable ignored = onClose("DROP TABLE " + tableName)) { // create multiple data files, INSERT with multiple values would create only one file (if not partitioned) - if (locationPattern == TWO_TRAILING_SLASHES && getClass().getName().contains(".deltalake.")) { - // TODO (https://github.com/trinodb/trino/issues/17966): writes fail when Delta table is declared with location ending with two slashes - assertThatThrownBy(() -> query("INSERT INTO " + tableName + " VALUES (1, 'one')")) - .hasMessageStartingWith("Location does not have parent: "); - return; - } assertUpdate("INSERT INTO " + tableName + " VALUES (1, 'one')", 1); assertUpdate("INSERT INTO " + tableName + " VALUES (2, 'a//double_slash')", 1); assertUpdate("INSERT INTO " + tableName + " VALUES (3, 'a%percent')", 1);