Support updates when Delta table location has two trailing slashes

lib/trino-filesystem/src/main/java/io/trino/filesystem/Location.java

@@ -178,6 +178,20 @@ public String fileName()
         return path.substring(path.lastIndexOf('/') + 1);
     }
 
+    /**
+     * Returns a new location with the same parent directory as the current location,
+     * but with the filename corresponding to the specified name.
+     * The location must be a valid file location.
+     */
+    public Location sibling(String name)
+    {
+        requireNonNull(name, "name is null");
+        checkArgument(!name.isEmpty(), "name is empty");
+        verifyValidFileLocation();
+
+        return this.withPath(location.substring(0, location.lastIndexOf('/') + 1) + name, path.substring(0, path.lastIndexOf('/') + 1) + name);
+    }
+
     /**
      * Creates a new location with all characters removed after the last slash in the path.
      * This should only be used once, as recursive calls for blob paths may lead to incorrect results.

lib/trino-filesystem/src/test/java/io/trino/filesystem/TestLocation.java

@@ -362,6 +362,49 @@ private static void assertFileName(String locationString, String fileName)
         assertThat(location.fileName()).isEqualTo(fileName);
     }
 
+    @Test
+    void testSibling()
+    {
+        assertSiblingFailure("/", "sibling", IllegalStateException.class, "File location must contain a path: /");
+        assertSiblingFailure("//", "sibling", IllegalStateException.class, "File location must contain a path: /");
+        assertSiblingFailure("file:/", "sibling", IllegalStateException.class, "File location must contain a path: file:/");
+        assertSiblingFailure("file://", "sibling", IllegalStateException.class, "File location must contain a path: file://");
+        assertSiblingFailure("file:///", "sibling", IllegalStateException.class, "File location must contain a path: file:///");
+        assertSiblingFailure("s3://bucket/", "sibling", IllegalStateException.class, "File location must contain a path: s3://bucket/");
+        assertSiblingFailure("scheme://userInfo@host/path/", "sibling", IllegalStateException.class, "File location cannot end with '/'");
+
+        assertSiblingFailure("scheme://userInfo@host/path/filename", null, NullPointerException.class, "name is null");
+        assertSiblingFailure("scheme://userInfo@host/path/filename", "", IllegalArgumentException.class, "name is empty");
+
+        assertSibling("scheme://userInfo@host/path/name", "sibling", "scheme://userInfo@host/path/sibling");
+        assertSibling("scheme://userInfo@host/path//name", "sibling", "scheme://userInfo@host/path//sibling");
+        assertSibling("scheme://userInfo@host/path///name", "sibling", "scheme://userInfo@host/path///sibling");
+        assertSibling("scheme://userInfo@host/level1/level2/name", "sibling", "scheme://userInfo@host/level1/level2/sibling");
+        assertSibling("scheme://userInfo@host/level1//level2/name", "sibling", "scheme://userInfo@host/level1//level2/sibling");
+
+        assertSibling("file:/path/name", "sibling", "file:/path/sibling");
+        assertSibling("s3://bucket/directory/filename with spaces", "sibling", "s3://bucket/directory/sibling");
+        assertSibling("/path/name", "sibling", "/path/sibling");
+        assertSibling("/name", "sibling", "/sibling");
+    }
+
+    private static void assertSiblingFailure(String locationString, String siblingName, Class<?> exceptionClass, String exceptionMessage)
+    {
+        assertThatThrownBy(() -> Location.of(locationString).sibling(siblingName))
+                .isInstanceOf(exceptionClass)
+                .hasMessageContaining(exceptionMessage);
+    }
+
+    private static void assertSibling(String locationString, String siblingName, String expectedLocationString)
+    {
+        // fileName method only works with valid file locations
+        Location location = Location.of(locationString);
+        location.verifyValidFileLocation();
+        Location siblingLocation = location.sibling(siblingName);
+
+        assertLocation(siblingLocation, Location.of(expectedLocationString));
+    }
+
     @Test
     void testParentDirectory()
     {

plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeMergeSink.java

@@ -330,7 +330,7 @@ private List<Slice> rewriteFile(String sourcePath, FileDeletion deletion)
             Location sourceLocation = Location.of(sourcePath);
             String sourceRelativePath = relativePath(tablePath, sourcePath);
 
-            Location targetLocation = sourceLocation.parentDirectory().appendPath(session.getQueryId() + "_" + randomUUID());
+            Location targetLocation = sourceLocation.sibling(session.getQueryId() + "_" + randomUUID());
             String targetRelativePath = relativePath(tablePath, targetLocation.toString());
             FileWriter fileWriter = createParquetFileWriter(targetLocation, dataColumns);
 

plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/transactionlog/writer/S3NativeTransactionLogSynchronizer.java

@@ -79,7 +79,7 @@ public boolean isUnsafe()
     public void write(ConnectorSession session, String clusterId, Location newLogEntryPath, byte[] entryContents)
     {
         TrinoFileSystem fileSystem = fileSystemFactory.create(session);
-        Location locksDirectory = newLogEntryPath.parentDirectory().appendPath(LOCK_DIRECTORY);
+        Location locksDirectory = newLogEntryPath.sibling(LOCK_DIRECTORY);
         String newEntryFilename = newLogEntryPath.fileName();
         Optional<LockInfo> myLockInfo = Optional.empty();
 

plugin/trino-hive/src/main/java/io/trino/plugin/hive/orc/OriginalFilesUtils.java

@@ -54,7 +54,7 @@ public static long getPrecedingRowCount(
         long rowCount = 0;
         for (OriginalFileInfo originalFileInfo : originalFileInfos) {
             if (originalFileInfo.getName().compareTo(splitPath.fileName()) < 0) {
-                Location path = splitPath.parentDirectory().appendPath(originalFileInfo.getName());
+                Location path = splitPath.sibling(originalFileInfo.getName());
                 TrinoInputFile inputFile = fileSystemFactory.create(identity)
                         .newInputFile(path, originalFileInfo.getFileSize());
                 rowCount += getRowsInFile(inputFile, options, stats);

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