From 986e3ca4c26544012c104e964945ff9d59259e70 Mon Sep 17 00:00:00 2001 From: Konrad Dziedzic Date: Wed, 11 Oct 2023 22:23:41 +0200 Subject: [PATCH] Fix tests failing due to lowering restrictions on s3 paths --- .../glue/TestDeltaS3AndGlueMetastoreTest.java | 7 +++-- .../hive/BaseS3AndGlueMetastoreTest.java | 4 ++- .../hive/TestHiveS3AndGlueMetastoreTest.java | 29 ++----------------- .../TestIcebergS3AndGlueMetastoreTest.java | 28 ++---------------- 4 files changed, 13 insertions(+), 55 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 cc12d940b2ee..880ccb0ac3fe 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 @@ -21,6 +21,7 @@ import java.nio.file.Path; import java.util.Set; +import java.util.regex.Pattern; import java.util.stream.Collectors; import static com.google.common.collect.Iterables.getOnlyElement; @@ -61,7 +62,7 @@ protected void validateDataFiles(String partitionColumn, String tableName, Strin { String locationDirectory = location.endsWith("/") ? location : location + "/"; String partitionPart = partitionColumn.isEmpty() ? "" : partitionColumn + "=[a-z0-9]+/"; - assertThat(dataFile).matches("^" + locationDirectory + partitionPart + "[a-zA-Z0-9_-]+$"); + assertThat(dataFile).matches("^" + Pattern.quote(locationDirectory) + partitionPart + "[a-zA-Z0-9_-]+$"); verifyPathExist(dataFile); }); } @@ -72,11 +73,11 @@ protected void validateMetadataFiles(String location) String locationDirectory = location.endsWith("/") ? location : location + "/"; getAllMetadataDataFilesFromTableDirectory(location).forEach(metadataFile -> { - assertThat(metadataFile).matches("^" + locationDirectory + "_delta_log/[0-9]+.json$"); + assertThat(metadataFile).matches("^" + Pattern.quote(locationDirectory) + "_delta_log/[0-9]+.json$"); verifyPathExist(metadataFile); }); - assertThat(getExtendedStatisticsFileFromTableDirectory(location)).matches("^" + locationDirectory + "_delta_log/_trino_meta/extended_stats.json$"); + assertThat(getExtendedStatisticsFileFromTableDirectory(location)).matches("^" + Pattern.quote(locationDirectory) + "_delta_log/_trino_meta/extended_stats.json$"); } @Override 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 578627ed2c6d..f43d7a7f72ca 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 @@ -137,7 +137,7 @@ public void testBasicOperationsWithProvidedSchemaLocation(boolean partitioned, L 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]+"; + String expectedTableLocationPattern = Pattern.quote(schemaLocation.endsWith("/") ? schemaLocation : schemaLocation + "/") + tableName + "-[a-z0-9]+"; actualTableLocation = getTableLocation(qualifiedTableName); assertThat(actualTableLocation).matches(expectedTableLocationPattern); @@ -323,6 +323,8 @@ protected enum LocationPattern DOUBLE_SLASH("s3://%s/%s//double_slash/%s"), TRIPLE_SLASH("s3://%s/%s///triple_slash/%s"), PERCENT("s3://%s/%s/a%%percent/%s"), + HASH("s3://%s/%s/a#hash/%s"), + QUESTION_MARK("s3://%s/%s/a?question_mark/%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 bc740b467222..16410b8a796e 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 @@ -25,6 +25,7 @@ import java.util.HashSet; import java.util.Optional; import java.util.Set; +import java.util.regex.Pattern; import static io.trino.plugin.hive.BaseS3AndGlueMetastoreTest.LocationPattern.DOUBLE_SLASH; import static io.trino.plugin.hive.BaseS3AndGlueMetastoreTest.LocationPattern.TRIPLE_SLASH; @@ -89,7 +90,7 @@ protected void validateDataFiles(String partitionColumn, String tableName, Strin { String locationDirectory = location.endsWith("/") ? location : location + "/"; String partitionPart = partitionColumn.isEmpty() ? "" : partitionColumn + "=[a-z0-9]+/"; - assertThat(dataFile).matches("^" + locationDirectory + partitionPart + "[a-zA-Z0-9_-]+$"); + assertThat(dataFile).matches("^" + Pattern.quote(locationDirectory) + partitionPart + "[a-zA-Z0-9_-]+$"); verifyPathExist(dataFile); }); } @@ -186,7 +187,7 @@ 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) + String expectedTableLocation = Pattern.quote((schemaLocation.endsWith("/") ? schemaLocation : schemaLocation + "/") + tableName) // Hive normalizes repeated slashes .replaceAll("(? assertUpdate("CREATE TABLE " + tableName + "(col_str varchar, col_int integer) WITH (external_location = '" + location + "')")) - .hasMessageContaining("External location is not a valid file system URI") - .hasStackTraceContaining("Fragment is not allowed in a file system location"); - } - - @Test - public void testCtasWithIncorrectLocation() - { - String tableName = "test_ctas_with_incorrect_location_" + randomNameSuffix(); - String location = "s3://%s/%s/a#hash/%s".formatted(bucketName, schemaName, tableName); - - assertThatThrownBy(() -> assertUpdate("CREATE TABLE " + tableName + "(col_str, col_int)" + - " WITH (external_location = '" + location + "')" + - " AS VALUES ('str1', 1)")) - .hasMessageContaining("External location is not a valid file system URI") - .hasStackTraceContaining("Fragment is not allowed in a file system location"); - } - @Test public void testSchemaNameEscape() { 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 ac1a6720a96d..65a03a866ed6 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 @@ -22,13 +22,13 @@ import java.nio.file.Path; import java.util.Set; +import java.util.regex.Pattern; import java.util.stream.Collectors; 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 @@ -60,7 +60,7 @@ protected void validateDataFiles(String partitionColumn, String tableName, Strin { String locationDirectory = location.endsWith("/") ? location : location + "/"; String partitionPart = partitionColumn.isEmpty() ? "" : partitionColumn + "=[a-z0-9]+/"; - assertThat(dataFile).matches("^" + locationDirectory + "data/" + partitionPart + "[a-zA-Z0-9_-]+.(orc|parquet)$"); + assertThat(dataFile).matches("^" + Pattern.quote(locationDirectory) + "data/" + partitionPart + "[a-zA-Z0-9_-]+.(orc|parquet)$"); verifyPathExist(dataFile); }); } @@ -71,7 +71,7 @@ protected void validateMetadataFiles(String location) getAllMetadataDataFilesFromTableDirectory(location).forEach(metadataFile -> { String locationDirectory = location.endsWith("/") ? location : location + "/"; - assertThat(metadataFile).matches("^" + locationDirectory + "metadata/[a-zA-Z0-9_-]+.(avro|metadata.json|stats)$"); + assertThat(metadataFile).matches("^" + Pattern.quote(locationDirectory) + "metadata/[a-zA-Z0-9_-]+.(avro|metadata.json|stats)$"); verifyPathExist(metadataFile); }); } @@ -134,26 +134,4 @@ public void testAnalyzeWithProvidedTableLocation(boolean partitioned, LocationPa assertQuery("SHOW STATS FOR " + tableName, expectedStatistics); } } - - @Test - public void testCreateTableWithIncorrectLocation() - { - String tableName = "test_create_table_with_incorrect_location_" + randomNameSuffix(); - String location = "s3://%s/%s/a#hash/%s".formatted(bucketName, schemaName, tableName); - - assertThatThrownBy(() -> assertUpdate("CREATE TABLE " + tableName + " (key integer, value varchar) WITH (location = '" + location + "')")) - .hasMessageContaining("Fragment is not allowed in a file system location"); - } - - @Test - public void testCtasWithIncorrectLocation() - { - String tableName = "test_create_table_with_incorrect_location_" + randomNameSuffix(); - String location = "s3://%s/%s/a#hash/%s".formatted(bucketName, schemaName, tableName); - - assertThatThrownBy(() -> assertUpdate("CREATE TABLE " + tableName + - " WITH (location = '" + location + "')" + - " AS SELECT * FROM tpch.tiny.nation")) - .hasMessageContaining("Fragment is not allowed in a file system location"); - } }