Skip to content

Commit

Permalink
Fix tests failing due to lowering restrictions on s3 paths
Browse files Browse the repository at this point in the history
  • Loading branch information
@homar @findepi
homar authored and findepi committed Oct 12, 2023
1 parent e026add commit 986e3ca
Show file tree
Hide file tree
Showing 4 changed files with 13 additions and 55 deletions.
7 changes: 4 additions & 3 deletions ...c/test/java/io/trino/plugin/deltalake/metastore/glue/TestDeltaS3AndGlueMetastoreTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
});
}
Expand All @@ -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
Expand Down
4 changes: 3 additions & 1 deletion plugin/trino-hive/src/test/java/io/trino/plugin/hive/BaseS3AndGlueMetastoreTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down Expand Up @@ -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 "),
/**/;
Expand Down
29 changes: 3 additions & 26 deletions plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHiveS3AndGlueMetastoreTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
});
}
Expand Down Expand Up @@ -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("(?<!(s3:))/+", "/");

Expand Down Expand Up @@ -278,30 +279,6 @@ public void testAnalyzeWithProvidedTableLocation(boolean partitioned, LocationPa
}
}

@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 + "(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()
{
Expand Down
28 changes: 3 additions & 25 deletions ...src/test/java/io/trino/plugin/iceberg/catalog/glue/TestIcebergS3AndGlueMetastoreTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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);
});
}
Expand All @@ -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);
});
}
Expand Down Expand Up @@ -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");
}
}

0 comments on commit 986e3ca

Please sign in to comment.