End in slash parent directory location

trinodb · Jul 7, 2023 · e022ef3 · e022ef3
1 parent e317bef
commit e022ef3
Show file tree

Hide file tree

Showing 3 changed files with 9 additions and 24 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,7 @@ public Location parentDirectory()
             return withPath(newLocation, "");
         }
 
-        String newPath = path.substring(0, lastIndexOfSlash);
+        String newPath = path.substring(0, lastIndexOfSlash + 1);
         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
@@ -377,24 +377,24 @@ void testParentDirectory()
         assertParentDirectoryFailure("scheme://userInfo@host//", "File location must contain a path: scheme://userInfo@host//");
         assertParentDirectoryFailure("scheme://userInfo@host:1234//", "File location must contain a path: 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: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///"));
+        assertParentDirectory("scheme://userInfo@host/path:/name", Location.of("scheme://userInfo@host/path:/"));
 
         assertParentDirectoryFailure("/", "File location must contain a path: /");
         assertParentDirectoryFailure("//", "File location must contain a path: //");
-        assertParentDirectory("/path/name", Location.of("/path"));
+        assertParentDirectory("/path/name", Location.of("/path/"));
         assertParentDirectory("/name", Location.of("/"));
 
         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///"));
+        assertParentDirectory("/path:/name", Location.of("/path:/"));
 
         // all valid file locations must have a parent directory
         // invalid file locations are tested in testVerifyFileLocation

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)");