diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8880d46d8165..008de783f436 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -281,14 +281,16 @@ jobs: plugin/trino-hive-hadoop2/bin/run_hive_s3_select_json_tests.sh fi fi - - name: Run Hive Glue Tests + - name: Run Hive AWS Tests env: AWS_ACCESS_KEY_ID: ${{ secrets.TRINO_AWS_ACCESS_KEY_ID }} AWS_SECRET_ACCESS_KEY: ${{ secrets.TRINO_AWS_SECRET_ACCESS_KEY }} AWS_REGION: us-east-2 + S3_BUCKET: "trino-ci-test" + S3_BUCKET_ENDPOINT: "s3.us-east-2.amazonaws.com" run: | if [ "${AWS_ACCESS_KEY_ID}" != "" ]; then - $MAVEN test ${MAVEN_TEST} -pl :trino-hive -P test-hive-glue + $MAVEN test ${MAVEN_TEST} -pl :trino-hive -P aws-tests fi - name: Run Hive Azure ABFS Access Key Tests if: matrix.config != 'config-empty' # Hive 1.x does not support Azure storage diff --git a/plugin/trino-hive/pom.xml b/plugin/trino-hive/pom.xml index eca5ce4e9d5a..375c5ca2cf13 100644 --- a/plugin/trino-hive/pom.xml +++ b/plugin/trino-hive/pom.xml @@ -509,6 +509,7 @@ **/TestHiveGlueMetastore.java + **/TestTrinoS3FileSystemAwsS3.java **/TestFullParquetReader.java @@ -546,7 +547,7 @@ - test-hive-glue + aws-tests @@ -555,6 +556,7 @@ **/TestHiveGlueMetastore.java + **/TestTrinoS3FileSystemAwsS3.java diff --git a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/s3/TrinoS3FileSystem.java b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/s3/TrinoS3FileSystem.java index a70f4c17abe0..5fca0edb52f9 100644 --- a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/s3/TrinoS3FileSystem.java +++ b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/s3/TrinoS3FileSystem.java @@ -124,6 +124,7 @@ import java.util.Iterator; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Optional; import java.util.OptionalInt; import java.util.Set; @@ -142,6 +143,7 @@ import static com.amazonaws.services.s3.model.StorageClass.Glacier; import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkPositionIndexes; +import static com.google.common.base.Preconditions.checkState; import static com.google.common.base.Strings.isNullOrEmpty; import static com.google.common.base.Strings.nullToEmpty; import static com.google.common.base.Throwables.throwIfInstanceOf; @@ -606,25 +608,81 @@ public boolean rename(Path src, Path dst) public boolean delete(Path path, boolean recursive) throws IOException { - try { - if (!directory(path)) { - return deleteObject(keyFromPath(path)); + String key = keyFromPath(path); + if (recursive) { + DeletePrefixResult deletePrefixResult; + try { + deletePrefixResult = deletePrefix(path); } + catch (AmazonClientException e) { + throw new IOException("Failed to delete paths with the prefix path " + path, e); + } + if (deletePrefixResult == DeletePrefixResult.NO_KEYS_FOUND) { + // If the provided key is not a "directory" prefix, attempt to delete the object with the specified key + deleteObject(key); + } + else if (deletePrefixResult == DeletePrefixResult.DELETE_KEYS_FAILURE) { + return false; + } + deleteObject(key + DIRECTORY_SUFFIX); } - catch (FileNotFoundException e) { - return false; + else { + Iterator listingsIterator = listObjects(path, OptionalInt.of(2), true); + Iterator objectKeysIterator = Iterators.concat(Iterators.transform(listingsIterator, TrinoS3FileSystem::keysFromRecursiveListing)); + if (objectKeysIterator.hasNext()) { + String childKey = objectKeysIterator.next(); + if (!Objects.equals(childKey, key + PATH_SEPARATOR) || objectKeysIterator.hasNext()) { + throw new IOException("Directory " + path + " is not empty"); + } + deleteObject(childKey); + } + else { + // Avoid deleting the bucket in case that the provided path points to the bucket root + if (!key.isEmpty()) { + deleteObject(key); + } + } + deleteObject(key + DIRECTORY_SUFFIX); } + return true; + } - if (!recursive) { - throw new IOException("Directory " + path + " is not empty"); + private DeletePrefixResult deletePrefix(Path prefix) + { + String bucketName = getBucketName(uri); + Iterator listings = listObjects(prefix, OptionalInt.empty(), true); + Iterator objectKeys = Iterators.concat(Iterators.transform(listings, TrinoS3FileSystem::keysFromRecursiveListing)); + Iterator> objectKeysBatches = Iterators.partition(objectKeys, DELETE_BATCH_SIZE); + if (!objectKeysBatches.hasNext()) { + return DeletePrefixResult.NO_KEYS_FOUND; } - for (FileStatus file : listStatus(path)) { - delete(file.getPath(), true); + boolean allKeysDeleted = true; + while (objectKeysBatches.hasNext()) { + String[] objectKeysBatch = objectKeysBatches.next().toArray(String[]::new); + try { + s3.deleteObjects(new DeleteObjectsRequest(bucketName) + .withKeys(objectKeysBatch) + .withRequesterPays(requesterPaysEnabled) + .withQuiet(true)); + } + catch (AmazonS3Exception e) { + log.debug(e, "Failed to delete objects from the bucket %s under the prefix '%s'", bucketName, prefix); + allKeysDeleted = false; + } } - deleteObject(keyFromPath(path) + DIRECTORY_SUFFIX); - return true; + return allKeysDeleted ? DeletePrefixResult.ALL_KEYS_DELETED : DeletePrefixResult.DELETE_KEYS_FAILURE; + } + + @VisibleForTesting + static Iterator keysFromRecursiveListing(ListObjectsV2Result listing) + { + checkState( + listing.getCommonPrefixes() == null || listing.getCommonPrefixes().isEmpty(), + "No common prefixes should be present when listing without a path delimiter"); + + return Iterators.transform(listing.getObjectSummaries().iterator(), S3ObjectSummary::getKey); } private boolean directory(Path path) @@ -701,12 +759,29 @@ public boolean isFilesOnly() { return (this == SHALLOW_FILES_ONLY || this == RECURSIVE_FILES_ONLY); } + + public boolean isRecursive() + { + return this == RECURSIVE_FILES_ONLY; + } } /** * List all objects rooted at the provided path. */ private Iterator listPath(Path path, OptionalInt initialMaxKeys, ListingMode mode) + { + Iterator listings = listObjects(path, initialMaxKeys, mode.isRecursive()); + + Iterator results = Iterators.concat(Iterators.transform(listings, this::statusFromListing)); + if (mode.isFilesOnly()) { + // Even recursive listing can still contain empty "directory" objects, must filter them out + results = Iterators.filter(results, LocatedFileStatus::isFile); + } + return results; + } + + private Iterator listObjects(Path path, OptionalInt initialMaxKeys, boolean recursive) { String key = keyFromPath(path); if (!key.isEmpty()) { @@ -716,12 +791,12 @@ private Iterator listPath(Path path, OptionalInt initialMaxKe ListObjectsV2Request request = new ListObjectsV2Request() .withBucketName(getBucketName(uri)) .withPrefix(key) - .withDelimiter(mode == ListingMode.RECURSIVE_FILES_ONLY ? null : PATH_SEPARATOR) + .withDelimiter(recursive ? null : PATH_SEPARATOR) .withMaxKeys(initialMaxKeys.isPresent() ? initialMaxKeys.getAsInt() : null) .withRequesterPays(requesterPaysEnabled); STATS.newListObjectsCall(); - Iterator listings = new AbstractSequentialIterator<>(s3.listObjectsV2(request)) + return new AbstractSequentialIterator<>(s3.listObjectsV2(request)) { @Override protected ListObjectsV2Result computeNext(ListObjectsV2Result previous) @@ -734,13 +809,6 @@ protected ListObjectsV2Result computeNext(ListObjectsV2Result previous) return s3.listObjectsV2(request); } }; - - Iterator results = Iterators.concat(Iterators.transform(listings, this::statusFromListing)); - if (mode.isFilesOnly()) { - // Even recursive listing can still contain empty "directory" objects, must filter them out - results = Iterators.filter(results, LocatedFileStatus::isFile); - } - return results; } private Iterator statusFromListing(ListObjectsV2Result listing) @@ -1896,4 +1964,11 @@ private static String getMd5AsBase64(byte[] data, int offset, int length) byte[] md5 = md5().hashBytes(data, offset, length).asBytes(); return Base64.getEncoder().encodeToString(md5); } + + private enum DeletePrefixResult + { + NO_KEYS_FOUND, + ALL_KEYS_DELETED, + DELETE_KEYS_FAILURE + } } diff --git a/plugin/trino-hive/src/test/java/io/trino/plugin/hive/s3/BaseTestTrinoS3FileSystemObjectStorage.java b/plugin/trino-hive/src/test/java/io/trino/plugin/hive/s3/BaseTestTrinoS3FileSystemObjectStorage.java new file mode 100644 index 000000000000..21292859df49 --- /dev/null +++ b/plugin/trino-hive/src/test/java/io/trino/plugin/hive/s3/BaseTestTrinoS3FileSystemObjectStorage.java @@ -0,0 +1,565 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.trino.plugin.hive.s3; + +import com.amazonaws.services.s3.AmazonS3; +import com.amazonaws.services.s3.model.ListObjectsV2Request; +import com.amazonaws.services.s3.model.ListObjectsV2Result; +import com.amazonaws.services.s3.model.ObjectMetadata; +import com.amazonaws.services.s3.model.PutObjectRequest; +import com.amazonaws.services.s3.model.S3ObjectSummary; +import com.google.common.net.MediaType; +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.fs.Path; +import org.testng.annotations.Test; + +import java.io.ByteArrayInputStream; +import java.io.InputStream; +import java.net.URI; +import java.util.ArrayList; +import java.util.List; + +import static com.google.common.collect.ImmutableList.toImmutableList; +import static io.airlift.testing.Closeables.closeAllSuppress; +import static io.trino.testing.TestingNames.randomNameSuffix; +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.testng.Assert.assertTrue; + +public abstract class BaseTestTrinoS3FileSystemObjectStorage +{ + private static final MediaType DIRECTORY_MEDIA_TYPE = MediaType.create("application", "x-directory"); + private static final String PATH_SEPARATOR = "/"; + private static final String DIRECTORY_SUFFIX = "_$folder$"; + + protected abstract String getBucketName(); + + protected abstract Configuration s3Configuration(); + + @Test + public void testDeleteRecursivelyMissingObjectPath() + throws Exception + { + String prefix = "test-delete-recursively-missing-object-" + randomNameSuffix(); + + try (TrinoS3FileSystem fs = createFileSystem()) { + // Follow Amazon S3 behavior if attempting to delete an object that does not exist + // and return a success message + assertTrue(fs.delete(new Path("s3://%s/%s".formatted(getBucketName(), prefix)), true)); + } + } + + @Test + public void testDeleteNonRecursivelyMissingObjectPath() + throws Exception + { + String prefix = "test-delete-non-recursively-missing-object-" + randomNameSuffix(); + + try (TrinoS3FileSystem fs = createFileSystem()) { + // Follow Amazon S3 behavior if attempting to delete an object that does not exist + // and return a success message + assertTrue(fs.delete(new Path("s3://%s/%s".formatted(getBucketName(), prefix)), false)); + } + } + + @Test + public void testDeleteRecursivelyObjectPath() + throws Exception + { + String prefix = "test-delete-recursively-object-" + randomNameSuffix(); + String prefixPath = "s3://%s/%s".formatted(getBucketName(), prefix); + + try (TrinoS3FileSystem fs = createFileSystem()) { + try { + String filename = "file.txt"; + String fileKey = "%s/%s".formatted(prefix, filename); + String filePath = "s3://%s/%s/%s".formatted(getBucketName(), prefix, filename); + fs.createNewFile(new Path(prefixPath, filename)); + List paths = listPaths(fs.getS3Client(), getBucketName(), prefix, true); + assertThat(paths).containsOnly(fileKey); + + assertTrue(fs.delete(new Path(filePath), true)); + + assertThat(listPaths(fs.getS3Client(), getBucketName(), prefix, true)).isEmpty(); + } + finally { + fs.delete(new Path(prefixPath), true); + } + } + } + + @Test + public void testDeleteNonRecursivelyObjectPath() + throws Exception + { + String prefix = "test-delete-non-recursively-object-" + randomNameSuffix(); + String prefixPath = "s3://%s/%s".formatted(getBucketName(), prefix); + + try (TrinoS3FileSystem fs = createFileSystem()) { + try { + String filename = "file.txt"; + String fileKey = "%s/%s".formatted(prefix, filename); + String filePath = "s3://%s/%s".formatted(getBucketName(), fileKey); + fs.createNewFile(new Path(prefixPath, filename)); + List paths = listPaths(fs.getS3Client(), getBucketName(), prefix, true); + assertThat(paths).containsOnly(fileKey); + + assertTrue(fs.delete(new Path(filePath), false)); + + assertThat(listPaths(fs.getS3Client(), getBucketName(), prefix, true)).isEmpty(); + } + finally { + fs.delete(new Path(prefixPath), true); + } + } + } + + @Test + public void testDeleteNonRecursivelyObjectNamePrefixingAnotherObjectName() + throws Exception + { + String prefix = "test-delete-non-recursively-object-delete-only-requested-object-" + randomNameSuffix(); + String prefixPath = "s3://%s/%s".formatted(getBucketName(), prefix); + + try (TrinoS3FileSystem fs = createFileSystem()) { + try { + fs.createNewFile(new Path(prefixPath, "foo")); + fs.createNewFile(new Path(prefixPath, "foobar")); + List paths = listPaths(fs.getS3Client(), getBucketName(), prefix, true); + assertThat(paths).containsOnly( + "%s/foo".formatted(prefix), + "%s/foobar".formatted(prefix)); + + assertTrue(fs.delete(new Path("s3://%s/%s/foo".formatted(getBucketName(), prefix)), false)); + + paths = listPaths(fs.getS3Client(), getBucketName(), prefix, true); + assertThat(paths).containsOnly( + "%s/foobar".formatted(prefix)); + } + finally { + fs.delete(new Path(prefixPath), true); + } + } + } + + @Test + public void testDeleteNonRecursivelyDirectoryNamePrefixingAnotherDirectoryName() + throws Exception + { + String prefix = "test-delete-non-recursively-object-delete-only-requested-directory-" + randomNameSuffix(); + String prefixPath = "s3://%s/%s".formatted(getBucketName(), prefix); + + try (TrinoS3FileSystem fs = createFileSystem()) { + try { + createDirectory(fs.getS3Client(), getBucketName(), "%s/foo".formatted(prefix)); + createDirectory(fs.getS3Client(), getBucketName(), "%s/foobar".formatted(prefix)); + List paths = listPaths(fs.getS3Client(), getBucketName(), prefix, true); + assertThat(paths).containsOnly( + "%s/foo/".formatted(prefix), + "%s/foobar/".formatted(prefix)); + + assertTrue(fs.delete(new Path("s3://%s/%s/foo".formatted(getBucketName(), prefix)), true)); + + paths = listPaths(fs.getS3Client(), getBucketName(), prefix, true); + assertThat(paths).containsOnly( + "%s/foobar/".formatted(prefix)); + } + finally { + fs.delete(new Path(prefixPath), true); + } + } + } + + @Test + public void testDeleteNonRecursivelyEmptyDirectory() + throws Exception + { + String prefix = "test-delete-non-recursively-empty-directory-" + randomNameSuffix(); + String prefixPath = "s3://%s/%s".formatted(getBucketName(), prefix); + + try (TrinoS3FileSystem fs = createFileSystem()) { + try { + createDirectory(fs.getS3Client(), getBucketName(), prefix); + List paths = listPaths(fs.getS3Client(), getBucketName(), prefix, false); + assertThat(paths).containsOnly(prefix + PATH_SEPARATOR); + + assertTrue(fs.delete(new Path(prefixPath), false)); + + assertThat(listPaths(fs.getS3Client(), getBucketName(), prefix, true)).isEmpty(); + } + finally { + fs.delete(new Path(prefixPath), true); + } + } + } + + @Test + public void testDeleteNonRecursivelyEmptyDirectoryWithAdditionalDirectorySuffixPlaceholder() + throws Exception + { + String directoryName = "test-delete-non-recursively-empty-directory-" + randomNameSuffix(); + String directoryPath = "s3://%s/%s".formatted(getBucketName(), directoryName); + + try (TrinoS3FileSystem fs = createFileSystem()) { + try { + createDirectory(fs.getS3Client(), getBucketName(), directoryName); + fs.createNewFile(new Path(directoryPath + DIRECTORY_SUFFIX)); + List paths = listPaths(fs.getS3Client(), getBucketName(), directoryName, true); + assertThat(paths).containsOnly( + directoryName + PATH_SEPARATOR, + directoryName + DIRECTORY_SUFFIX); + + assertTrue(fs.delete(new Path(directoryPath), false)); + + assertThat(listPaths(fs.getS3Client(), getBucketName(), directoryName, true)).isEmpty(); + } + finally { + fs.delete(new Path(directoryPath), true); + } + } + } + + @Test + public void testDeleteRecursivelyObjectNamePrefixingAnotherObjectName() + throws Exception + { + String prefix = "test-delete-recursively-object-delete-only-requested-object-" + randomNameSuffix(); + String prefixPath = "s3://%s/%s".formatted(getBucketName(), prefix); + + try (TrinoS3FileSystem fs = createFileSystem()) { + try { + fs.createNewFile(new Path(prefixPath, "foo")); + fs.createNewFile(new Path(prefixPath, "foobar")); + List paths = listPaths(fs.getS3Client(), getBucketName(), prefix, true); + assertThat(paths).containsOnly( + "%s/foo".formatted(prefix), + "%s/foobar".formatted(prefix)); + + assertTrue(fs.delete(new Path("s3://%s/%s/foo".formatted(getBucketName(), prefix)), true)); + + paths = listPaths(fs.getS3Client(), getBucketName(), prefix, true); + assertThat(paths).containsOnly( + "%s/foobar".formatted(prefix)); + } + finally { + fs.delete(new Path(prefixPath), true); + } + } + } + + @Test + public void testDeleteRecursivelyDirectoryNamePrefixingAnotherDirectoryName() + throws Exception + { + String prefix = "test-delete-recursively-object-delete-only-requested-directory-" + randomNameSuffix(); + String prefixPath = "s3://%s/%s".formatted(getBucketName(), prefix); + + try (TrinoS3FileSystem fs = createFileSystem()) { + try { + createDirectory(fs.getS3Client(), getBucketName(), "%s/foo".formatted(prefix)); + createDirectory(fs.getS3Client(), getBucketName(), "%s/foobar".formatted(prefix)); + List paths = listPaths(fs.getS3Client(), getBucketName(), prefix, true); + assertThat(paths).containsOnly( + "%s/foo/".formatted(prefix), + "%s/foobar/".formatted(prefix)); + + assertTrue(fs.delete(new Path("s3://%s/%s/foo".formatted(getBucketName(), prefix)), true)); + + paths = listPaths(fs.getS3Client(), getBucketName(), prefix, true); + assertThat(paths).containsOnly("%s/foobar/".formatted(prefix)); + } + finally { + fs.delete(new Path(prefixPath), true); + } + } + } + + @Test + public void testDeleteRecursivelyPrefixContainingMultipleObjectsPlain() + throws Exception + { + String prefix = "test-delete-recursively-path-multiple-objects-plain-" + randomNameSuffix(); + String prefixPath = "s3://%s/%s".formatted(getBucketName(), prefix); + + try (TrinoS3FileSystem fs = createFileSystem()) { + try { + String filename1 = "file1.txt"; + String filename2 = "file2.txt"; + fs.createNewFile(new Path(prefixPath, filename1)); + fs.createNewFile(new Path(prefixPath, filename2)); + List paths = listPaths(fs.getS3Client(), getBucketName(), prefix, true); + assertThat(paths).containsOnly( + "%s/%s".formatted(prefix, filename1), + "%s/%s".formatted(prefix, filename2)); + + assertTrue(fs.delete(new Path(prefixPath), true)); + + assertThat(listPaths(fs.getS3Client(), getBucketName(), prefix, true)).isEmpty(); + } + finally { + fs.delete(new Path(prefixPath), true); + } + } + } + + @Test + public void testDeleteRecursivelyDirectoryWithDeepHierarchy() + throws Exception + { + String prefix = "test-delete-recursively-directory-deep-hierarchy-" + randomNameSuffix(); + String prefixPath = "s3://%s/%s".formatted(getBucketName(), prefix); + + try (TrinoS3FileSystem fs = createFileSystem()) { + try { + String directoryKey = prefix + "/directory"; + String directoryPath = "s3://%s/%s".formatted(getBucketName(), directoryKey); + createDirectory(fs.getS3Client(), getBucketName(), directoryKey); + + String filename1 = "file1.txt"; + String filename2 = "file2.txt"; + String filename3 = "file3.txt"; + fs.createNewFile(new Path(directoryPath, filename1)); + fs.createNewFile(new Path(directoryPath, filename2)); + fs.createNewFile(new Path(directoryPath + "/dir3", filename3)); + createDirectory(fs.getS3Client(), getBucketName(), directoryKey + "/dir4"); + + assertTrue(fs.delete(new Path(directoryPath), true)); + + assertThat(listPaths(fs.getS3Client(), getBucketName(), prefix, true)).isEmpty(); + } + finally { + fs.delete(new Path(prefixPath), true); + } + } + } + + @Test + public void testDeleteRecursivelyEmptyDirectory() + throws Exception + { + String prefix = "test-delete-recursively-empty-directory-" + randomNameSuffix(); + String prefixPath = "s3://%s/%s".formatted(getBucketName(), prefix); + + try (TrinoS3FileSystem fs = createFileSystem()) { + try { + String directoryKey = prefix + "/directory"; + createDirectory(fs.getS3Client(), getBucketName(), directoryKey); + fs.createNewFile(new Path("s3://%s/%s%s".formatted(getBucketName(), directoryKey, DIRECTORY_SUFFIX))); + List paths = listPaths(fs.getS3Client(), getBucketName(), prefix, true); + assertThat(paths).containsOnly( + directoryKey + PATH_SEPARATOR, + directoryKey + DIRECTORY_SUFFIX); + + assertTrue(fs.delete(new Path(prefixPath + "/directory"), true)); + + assertThat(listPaths(fs.getS3Client(), getBucketName(), prefix, true)).isEmpty(); + } + finally { + fs.delete(new Path(prefixPath), true); + } + } + } + + @Test + public void testDeleteRecursivelyDirectoryWithObjectsAndDirectorySuffixPlaceholder() + throws Exception + { + String prefix = "test-delete-recursively-directory-multiple-objects-" + randomNameSuffix(); + String prefixPath = "s3://%s/%s".formatted(getBucketName(), prefix); + + try (TrinoS3FileSystem fs = createFileSystem()) { + try { + String directoryKey = prefix + "/directory"; + String directoryPath = "s3://%s/%s".formatted(getBucketName(), directoryKey); + createDirectory(fs.getS3Client(), getBucketName(), directoryKey); + fs.createNewFile(new Path(directoryPath + DIRECTORY_SUFFIX)); + + String filename1 = "file1.txt"; + String filename2 = "file2.txt"; + String filename3 = "file3.txt"; + fs.createNewFile(new Path(directoryPath, filename1)); + fs.createNewFile(new Path(directoryPath, filename2)); + fs.createNewFile(new Path(directoryPath + "/dir3", filename3)); + fs.createNewFile(new Path(directoryPath + "/dir3" + DIRECTORY_SUFFIX)); + createDirectory(fs.getS3Client(), getBucketName(), directoryKey + "/dir4"); + fs.createNewFile(new Path(directoryPath + "/dir4" + DIRECTORY_SUFFIX)); + + assertTrue(fs.delete(new Path(directoryPath), true)); + + assertThat(listPaths(fs.getS3Client(), getBucketName(), prefix, true)).isEmpty(); + } + finally { + fs.delete(new Path(prefixPath), true); + } + } + } + + @Test + public void testDeleteRecursivelyPrefixContainingDeepHierarchy() + throws Exception + { + String prefix = "test-delete-recursively-prefix-deep-hierarchy-" + randomNameSuffix(); + String prefixPath = "s3://%s/%s".formatted(getBucketName(), prefix); + + try (TrinoS3FileSystem fs = createFileSystem()) { + try { + String filename1 = "file1.txt"; + String filename2 = "file2.txt"; + String filename3 = "file3.txt"; + fs.createNewFile(new Path("s3://%s/%s/dir1".formatted(getBucketName(), prefix), filename1)); + fs.createNewFile(new Path("s3://%s/%s/dir2/dir22".formatted(getBucketName(), prefix), filename2)); + fs.createNewFile(new Path("s3://%s/%s/dir3/dir33/dir333".formatted(getBucketName(), prefix), filename3)); + List paths = listPaths(fs.getS3Client(), getBucketName(), prefix, true); + assertThat(paths).containsOnly( + "%s/dir1/%s".formatted(prefix, filename1), + "%s/dir2/dir22/%s".formatted(prefix, filename2), + "%s/dir3/dir33/dir333/%s".formatted(prefix, filename3)); + + assertTrue(fs.delete(new Path(prefixPath), true)); + + assertThat(listPaths(fs.getS3Client(), getBucketName(), prefix, true)).isEmpty(); + } + finally { + fs.delete(new Path(prefixPath), true); + } + } + } + + @Test + public void testDeleteNonRecursivelyNonEmptyDirectory() + throws Exception + { + String prefix = "test-illegal-delete-non-recursively-directory-non-empty-" + randomNameSuffix(); + String prefixPath = "s3://%s/%s".formatted(getBucketName(), prefix); + + try (TrinoS3FileSystem fs = createFileSystem()) { + try { + String directoryKey = prefix + "/directory"; + String directoryPath = "s3://%s/%s".formatted(getBucketName(), directoryKey); + createDirectory(fs.getS3Client(), getBucketName(), directoryKey); + + fs.createNewFile(new Path(directoryPath, "file1.txt")); + + assertThatThrownBy(() -> fs.delete(new Path(directoryPath), false)) + .hasMessage("Directory %s is not empty".formatted(directoryPath)); + + List paths = listPaths(fs.getS3Client(), getBucketName(), prefix, true); + assertThat(paths).containsOnly( + "%s/directory/".formatted(prefix), + "%s/directory/file1.txt".formatted(prefix)); + } + finally { + fs.delete(new Path(prefixPath), true); + } + } + } + + @Test + public void testDeleteNonRecursivelyNonEmptyPath() + throws Exception + { + String prefix = "test-illegal-delete-non-recursively-path-non-empty-" + randomNameSuffix(); + String prefixPath = "s3://%s/%s".formatted(getBucketName(), prefix); + + try (TrinoS3FileSystem fs = createFileSystem()) { + try { + fs.createNewFile(new Path(prefixPath, "file1.txt")); + + assertThatThrownBy(() -> fs.delete(new Path("s3://%s/%s".formatted(getBucketName(), prefix)), false)) + .hasMessage("Directory s3://%s/%s is not empty".formatted(getBucketName(), prefix)); + + List paths = listPaths(fs.getS3Client(), getBucketName(), prefix, true); + assertThat(paths).containsOnly( + "%s/file1.txt".formatted(prefix)); + } + finally { + fs.delete(new Path(prefixPath), true); + } + } + } + + @Test + public void testDeleteNonRecursivelyNonEmptyDeepPath() + throws Exception + { + String prefix = "test-illegal-delete-non-recursively-deep-path-non-empty-" + randomNameSuffix(); + String prefixPath = "s3://%s/%s".formatted(getBucketName(), prefix); + + try (TrinoS3FileSystem fs = createFileSystem()) { + try { + String filename1 = "file1.txt"; + String filename2 = "file2.txt"; + fs.createNewFile(new Path(prefixPath + "/dir1/", filename1)); + fs.createNewFile(new Path(prefixPath + "/dir2/", filename2)); + List paths = listPaths(fs.getS3Client(), getBucketName(), prefix, true); + assertThat(paths).containsOnly( + "%s/dir1/%s".formatted(prefix, filename1), + "%s/dir2/%s".formatted(prefix, filename2)); + + assertThatThrownBy(() -> fs.delete(new Path("s3://%s/%s".formatted(getBucketName(), prefix)), false)) + .hasMessage("Directory s3://%s/%s is not empty".formatted(getBucketName(), prefix)); + + paths = listPaths(fs.getS3Client(), getBucketName(), prefix, true); + assertThat(paths).containsOnly( + "%s/dir1/%s".formatted(prefix, filename1), + "%s/dir2/%s".formatted(prefix, filename2)); + } + finally { + fs.delete(new Path(prefixPath), true); + } + } + } + + protected TrinoS3FileSystem createFileSystem() + throws Exception + { + TrinoS3FileSystem fs = new TrinoS3FileSystem(); + try { + fs.initialize(new URI("s3://%s/".formatted(getBucketName())), s3Configuration()); + } + catch (Throwable e) { + closeAllSuppress(e, fs); + throw e; + } + return fs; + } + + protected static void createDirectory(AmazonS3 client, String bucketName, String key) + { + // create meta-data for your folder and set content-length to 0 + ObjectMetadata metadata = new ObjectMetadata(); + metadata.setContentLength(0); + metadata.setContentType(DIRECTORY_MEDIA_TYPE.toString()); + // create empty content + InputStream emptyContent = new ByteArrayInputStream(new byte[0]); + // create a PutObjectRequest passing the folder name suffixed by / + PutObjectRequest putObjectRequest = new PutObjectRequest(bucketName, key + PATH_SEPARATOR, emptyContent, metadata); + // send request to S3 to create folder + client.putObject(putObjectRequest); + } + + protected static List listPaths(AmazonS3 s3, String bucketName, String prefix, boolean recursive) + { + ListObjectsV2Request request = new ListObjectsV2Request() + .withBucketName(bucketName) + .withPrefix(prefix) + .withDelimiter(recursive ? null : PATH_SEPARATOR); + ListObjectsV2Result listing = s3.listObjectsV2(request); + + List paths = new ArrayList<>(); + paths.addAll(listing.getCommonPrefixes()); + paths.addAll(listing.getObjectSummaries().stream().map(S3ObjectSummary::getKey).collect(toImmutableList())); + return paths; + } +} diff --git a/plugin/trino-hive/src/test/java/io/trino/plugin/hive/s3/TestTrinoS3FileSystemAwsS3.java b/plugin/trino-hive/src/test/java/io/trino/plugin/hive/s3/TestTrinoS3FileSystemAwsS3.java new file mode 100644 index 000000000000..9801ea567be5 --- /dev/null +++ b/plugin/trino-hive/src/test/java/io/trino/plugin/hive/s3/TestTrinoS3FileSystemAwsS3.java @@ -0,0 +1,54 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.trino.plugin.hive.s3; + +import org.apache.hadoop.conf.Configuration; +import org.testng.annotations.BeforeClass; + +import static io.trino.hadoop.ConfigurationInstantiator.newEmptyConfiguration; +import static java.util.Objects.requireNonNull; + +/** + * Tests file system operations on AWS S3 storage. + *

+ * Requires AWS credentials, which can be provided any way supported by the DefaultProviderChain + * See https://docs.aws.amazon.com/sdk-for-java/v1/developer-guide/credentials.html#credentials-default + */ +public class TestTrinoS3FileSystemAwsS3 + extends BaseTestTrinoS3FileSystemObjectStorage +{ + private String bucketName; + private String s3Endpoint; + + @BeforeClass + public void setup() + { + bucketName = requireNonNull(System.getenv("S3_BUCKET"), "Environment S3_BUCKET was not set"); + s3Endpoint = requireNonNull(System.getenv("S3_BUCKET_ENDPOINT"), "Environment S3_BUCKET_ENDPOINT was not set"); + } + + @Override + protected String getBucketName() + { + return bucketName; + } + + @Override + protected Configuration s3Configuration() + { + Configuration config = newEmptyConfiguration(); + config.set("fs.s3.endpoint", s3Endpoint); + return config; + } +} diff --git a/plugin/trino-hive/src/test/java/io/trino/plugin/hive/s3/TestTrinoS3FileSystemMinio.java b/plugin/trino-hive/src/test/java/io/trino/plugin/hive/s3/TestTrinoS3FileSystemMinio.java new file mode 100644 index 000000000000..3a52756ce0fb --- /dev/null +++ b/plugin/trino-hive/src/test/java/io/trino/plugin/hive/s3/TestTrinoS3FileSystemMinio.java @@ -0,0 +1,165 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.trino.plugin.hive.s3; + +import com.amazonaws.services.s3.AmazonS3; +import com.google.common.collect.ImmutableMap; +import io.trino.testing.containers.Minio; +import io.trino.testing.minio.MinioClient; +import io.trino.util.AutoCloseableCloser; +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.fs.Path; +import org.testng.annotations.AfterClass; +import org.testng.annotations.BeforeClass; +import org.testng.annotations.Test; + +import java.net.URI; + +import static io.trino.hadoop.ConfigurationInstantiator.newEmptyConfiguration; +import static io.trino.testing.TestingNames.randomNameSuffix; +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.testng.Assert.assertTrue; + +public class TestTrinoS3FileSystemMinio + extends BaseTestTrinoS3FileSystemObjectStorage +{ + public static final String MINIO_ACCESS_KEY = "accesskey"; + public static final String MINIO_SECRET_KEY = "secretkey"; + + private final String bucketName = "trino-ci-test"; + + private Minio minio; + + private MinioClient minioClient; + + @BeforeClass + public void setup() + throws Exception + { + minio = Minio.builder() + .withEnvVars(ImmutableMap.builder() + .put("MINIO_ACCESS_KEY", MINIO_ACCESS_KEY) + .put("MINIO_SECRET_KEY", MINIO_SECRET_KEY) + .buildOrThrow()) + .build(); + minio.start(); + + minioClient = minio.createMinioClient(); + minio.createBucket(bucketName); + } + + @AfterClass(alwaysRun = true) + public void tearDown() + throws Exception + { + try (AutoCloseableCloser closer = AutoCloseableCloser.create()) { + closer.register(minio); + closer.register(minioClient); + } + minioClient = null; + minio = null; + } + + @Override + protected String getBucketName() + { + return bucketName; + } + + @Override + protected Configuration s3Configuration() + { + Configuration config = newEmptyConfiguration(); + config.set("trino.s3.endpoint", minio.getMinioAddress()); + config.set("trino.s3.access-key", MINIO_ACCESS_KEY); + config.set("trino.s3.secret-key", MINIO_SECRET_KEY); + config.set("trino.s3.path-style-access", "true"); + + return config; + } + + @Test + public void testDeleteNonRecursivelyEmptyBucketRoot() + throws Exception + { + String testBucketName = "trino-delete-bucket-root-empty" + randomNameSuffix(); + minioClient.makeBucket(testBucketName); + String testBucketPath = "s3://%s/".formatted(testBucketName); + try (TrinoS3FileSystem fs = new TrinoS3FileSystem()) { + fs.initialize(new URI(testBucketPath), s3Configuration()); + + AmazonS3 s3 = fs.getS3Client(); + + assertThat(listPaths(s3, testBucketName, "", true)).isEmpty(); + + fs.delete(new Path(testBucketPath), false); + + assertThat(listPaths(s3, testBucketName, "", true)).isEmpty(); + } + } + + @Test + public void testDeleteNonRecursivelyNonEmptyBucketRoot() + throws Exception + { + String testBucketName = "trino-delete-bucket-root-non-empty" + randomNameSuffix(); + minioClient.makeBucket(testBucketName); + String testBucketPath = "s3://%s/".formatted(testBucketName); + try (TrinoS3FileSystem fs = new TrinoS3FileSystem()) { + fs.initialize(new URI(testBucketPath), s3Configuration()); + + AmazonS3 s3 = fs.getS3Client(); + fs.createNewFile(new Path("s3://%s/file1.txt".formatted(testBucketName))); + String directory2Path = testBucketPath + "directory2"; + createDirectory(fs.getS3Client(), testBucketName, "directory2"); + String filename2 = "file2.txt"; + fs.createNewFile(new Path(directory2Path, filename2)); + + assertThat(listPaths(s3, testBucketName, "", true)) + .containsOnly("file1.txt", "directory2/", "directory2/file2.txt"); + + assertThatThrownBy(() -> fs.delete(new Path(testBucketPath), false)) + .hasMessage("Directory %s is not empty".formatted(testBucketPath)); + + assertThat(listPaths(s3, testBucketName, "", true)) + .containsOnly("file1.txt", "directory2/", "directory2/file2.txt"); + } + } + + @Test + public void testDeleteRecursivelyBucketRoot() + throws Exception + { + String testBucketName = "trino-delete-recursive-bucket-root" + randomNameSuffix(); + minioClient.makeBucket(testBucketName); + String testBucketPath = "s3://" + testBucketName; + try (TrinoS3FileSystem fs = new TrinoS3FileSystem()) { + fs.initialize(new URI(testBucketPath), s3Configuration()); + + AmazonS3 s3 = fs.getS3Client(); + fs.createNewFile(new Path("s3://%s/file1.txt".formatted(testBucketName))); + String directory2Path = testBucketPath + "/directory2"; + createDirectory(fs.getS3Client(), testBucketName, "directory2"); + fs.createNewFile(new Path(directory2Path, "file2.txt")); + + assertThat(listPaths(s3, testBucketName, "", true)) + .containsOnly("file1.txt", "directory2/", "directory2/file2.txt"); + + assertTrue(fs.delete(new Path(testBucketPath + Path.SEPARATOR), true)); + + assertThat(listPaths(s3, testBucketName, "", true)).isEmpty(); + } + } +} diff --git a/testing/trino-product-tests-launcher/src/main/java/io/trino/tests/product/launcher/suite/SuiteDeltaLakeDatabricks.java b/testing/trino-product-tests-launcher/src/main/java/io/trino/tests/product/launcher/suite/SuiteDeltaLakeDatabricks.java index ff8b469e3338..7b1c8b8f7bef 100644 --- a/testing/trino-product-tests-launcher/src/main/java/io/trino/tests/product/launcher/suite/SuiteDeltaLakeDatabricks.java +++ b/testing/trino-product-tests-launcher/src/main/java/io/trino/tests/product/launcher/suite/SuiteDeltaLakeDatabricks.java @@ -30,8 +30,6 @@ protected String[] getExcludedTests() // AWS Glue does not support table renames "io.trino.tests.product.deltalake.TestHiveAndDeltaLakeRedirect.testDeltaToHiveAlterTable", "io.trino.tests.product.deltalake.TestHiveAndDeltaLakeRedirect.testHiveToDeltaAlterTable", - // TODO https://github.com/trinodb/trino/issues/13017 - "io.trino.tests.product.deltalake.TestDeltaLakeDropTableCompatibility.testCreateManagedTableInDeltaDropTableInTrino" }; } } diff --git a/testing/trino-product-tests/src/main/java/io/trino/tests/product/deltalake/TestDeltaLakeDropTableCompatibility.java b/testing/trino-product-tests/src/main/java/io/trino/tests/product/deltalake/TestDeltaLakeDropTableCompatibility.java index 26bbd40404d8..3f090492c0db 100644 --- a/testing/trino-product-tests/src/main/java/io/trino/tests/product/deltalake/TestDeltaLakeDropTableCompatibility.java +++ b/testing/trino-product-tests/src/main/java/io/trino/tests/product/deltalake/TestDeltaLakeDropTableCompatibility.java @@ -63,6 +63,7 @@ public static Object[][] engineConfigurations() {TRINO, DELTA, true}, {TRINO, DELTA, false}, {DELTA, TRINO, true}, + {DELTA, TRINO, false}, {DELTA, DELTA, true}, {DELTA, DELTA, false}, }; @@ -75,14 +76,6 @@ public void testDropTable(Engine creator, Engine dropper, boolean explicitLocati testDropTableAccuracy(creator, dropper, explicitLocation); } - @Test(groups = {DELTA_LAKE_DATABRICKS, DELTA_LAKE_OSS, PROFILE_SPECIFIC_TESTS}) - @Flaky(issue = DATABRICKS_COMMUNICATION_FAILURE_ISSUE, match = DATABRICKS_COMMUNICATION_FAILURE_MATCH) - public void testCreateManagedTableInDeltaDropTableInTrino() - { - //TODO Integrate this method into `engineConfigurations()` data provider method after dealing with https://github.com/trinodb/trino/issues/13017 - testDropTableAccuracy(DELTA, TRINO, false); - } - private void testDropTableAccuracy(Engine creator, Engine dropper, boolean explicitLocation) { String schemaName = "test_schema_with_location_" + randomNameSuffix();