Add special handling for multiple trailing slashes in the parent dire…

…ctory
findinpath · Jul 6, 2023 · e6ad76f · e6ad76f
1 parent f838546
commit e6ad76f
Show file tree

Hide file tree

Showing 3 changed files with 8 additions and 20 deletions.
diff --git a/lib/trino-filesystem/src/main/java/io/trino/filesystem/Location.java b/lib/trino-filesystem/src/main/java/io/trino/filesystem/Location.java
@@ -197,7 +197,10 @@ public Location parentDirectory()
             return withPath(newLocation, "");
         }
 
-        String newPath = path.substring(0, lastIndexOfSlash);
+        // Special handling for the case where the parent path contains multiple trailing slashes
+        // The parent directory for `s3://bucket/directory//file` is `s3://bucket/directory//`
+        int parentDirectoryEndIndex = (lastIndexOfSlash > 0 && path.charAt(lastIndexOfSlash - 1) == '/') ? lastIndexOfSlash + 1 : lastIndexOfSlash;
+        String newPath = path.substring(0, parentDirectoryEndIndex);
         String newLocation = location.substring(0, location.length() - (path.length() - newPath.length()));
         return withPath(newLocation, newPath);
     }

diff --git a/lib/trino-filesystem/src/test/java/io/trino/filesystem/TestLocation.java b/lib/trino-filesystem/src/test/java/io/trino/filesystem/TestLocation.java
@@ -380,8 +380,8 @@ void testParentDirectory()
         assertParentDirectory("scheme://userInfo@host/path/name", Location.of("scheme://userInfo@host/path"));
         assertParentDirectory("scheme://userInfo@host:1234/name", Location.of("scheme://userInfo@host:1234/"));
 
-        assertParentDirectory("scheme://userInfo@host/path//name", Location.of("scheme://userInfo@host/path/"));
-        assertParentDirectory("scheme://userInfo@host/path///name", Location.of("scheme://userInfo@host/path//"));
+        assertParentDirectory("scheme://userInfo@host/path//name", Location.of("scheme://userInfo@host/path//"));
+        assertParentDirectory("scheme://userInfo@host/path///name", Location.of("scheme://userInfo@host/path///"));
         assertParentDirectory("scheme://userInfo@host/path:/name", Location.of("scheme://userInfo@host/path:"));
 
         assertParentDirectoryFailure("/", "File location must contain a path: /");
@@ -392,8 +392,8 @@ void testParentDirectory()
         assertParentDirectoryFailure("/path/name/", "File location cannot end with '/': /path/name/");
         assertParentDirectoryFailure("/name/", "File location cannot end with '/': /name/");
 
-        assertParentDirectory("/path//name", Location.of("/path/"));
-        assertParentDirectory("/path///name", Location.of("/path//"));
+        assertParentDirectory("/path//name", Location.of("/path//"));
+        assertParentDirectory("/path///name", Location.of("/path///"));
         assertParentDirectory("/path:/name", Location.of("/path:"));
 
         // all valid file locations must have a parent directory

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
@@ -36,7 +36,6 @@
 
 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;
@@ -45,7 +44,6 @@
 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
@@ -109,12 +107,6 @@ public void testBasicOperationsWithProvidedTableLocation(boolean partitioned, Lo
             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)");
 
@@ -186,13 +178,6 @@ public void testMergeWithProvidedTableLocation(boolean partitioned, LocationPatt
                     " 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)");