Skip to content

Commit

Permalink
Fix Delta INSERT when schema OR table location ends with two slashes
Browse files Browse the repository at this point in the history
  • Loading branch information
@findepi
findepi committed Jun 21, 2023
1 parent e456342 commit 05b9fc4
Show file tree
Hide file tree
Showing 2 changed files with 14 additions and 27 deletions.
3 changes: 1 addition & 2 deletions plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeMetadata.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,6 @@
import static com.google.common.collect.Sets.difference;
import static com.google.common.primitives.Ints.max;
import static io.trino.filesystem.Locations.appendPath;
import static io.trino.filesystem.Locations.getParent;
import static io.trino.plugin.deltalake.DataFileInfo.DataFileType.DATA;
import static io.trino.plugin.deltalake.DeltaLakeAnalyzeProperties.AnalyzeMode.FULL_REFRESH;
import static io.trino.plugin.deltalake.DeltaLakeAnalyzeProperties.AnalyzeMode.INCREMENTAL;
Expand Down Expand Up @@ -1903,7 +1902,7 @@ private void checkWriteAllowed(ConnectorSession session, DeltaLakeTableHandle ta
private boolean allowWrite(ConnectorSession session, DeltaLakeTableHandle tableHandle)
{
try {
String tableMetadataDirectory = appendPath(getParent(tableHandle.getLocation()), tableHandle.getTableName());
String tableMetadataDirectory = getTransactionLogDir(tableHandle.getLocation());
boolean requiresOptIn = transactionLogWriterFactory.newWriter(session, tableMetadataDirectory).isUnsafe();
return !requiresOptIn || unsafeWritesEnabled;
}
Expand Down
38 changes: 13 additions & 25 deletions 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 @@ -106,15 +106,15 @@ public void testBasicOperationsWithProvidedTableLocation(boolean partitioned, Lo
assertQuery("SELECT * FROM " + tableName, "VALUES ('str1', 1), ('str2', 2), ('str3', 3)");
actualTableLocation = validateTableLocation(tableName, location);

if (locationPattern == TWO_TRAILING_SLASHES && getClass().getName().contains(".deltalake.")) {
// TODO (https://github.com/trinodb/trino/issues/17966): writes fail when Delta table is declared with location ending with two slashes
assertThatThrownBy(() -> query("INSERT INTO " + tableName + " VALUES ('str4', 4)"))
.hasMessageStartingWith("Location does not have parent: ");
return;
}
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)");

Expand Down Expand Up @@ -149,12 +149,6 @@ public void testBasicOperationsWithProvidedSchemaLocation(boolean partitioned, L
actualTableLocation = getTableLocation(qualifiedTableName);
assertThat(actualTableLocation).matches(expectedTableLocationPattern);

if (locationPattern == TWO_TRAILING_SLASHES && getClass().getName().contains(".deltalake.")) {
// TODO (https://github.com/trinodb/trino/issues/17966): writes fail when Delta table is declared within schema with location ending with two slashes
assertThatThrownBy(() -> query("INSERT INTO " + qualifiedTableName + " (col_str, col_int) VALUES ('str1', 1), ('str2', 2), ('str3', 3)"))
.hasMessageStartingWith("Location does not have parent: ");
return;
}
assertUpdate("INSERT INTO " + qualifiedTableName + " (col_str, col_int) VALUES ('str1', 1), ('str2', 2), ('str3', 3)", 3);
assertQuery("SELECT col_str, col_int FROM " + qualifiedTableName, "VALUES ('str1', 1), ('str2', 2), ('str3', 3)");

Expand Down Expand Up @@ -188,17 +182,17 @@ public void testMergeWithProvidedTableLocation(boolean partitioned, LocationPatt
actualTableLocation = validateTableLocation(tableName, location);
assertQuery("SELECT * FROM " + tableName, "VALUES ('str1', 1), ('str2', 2), ('str3', 3)");

if (locationPattern == TWO_TRAILING_SLASHES && getClass().getName().contains(".deltalake.")) {
// TODO (https://github.com/trinodb/trino/issues/17966): writes fail when Delta table is declared with location ending with two slashes
assertThatThrownBy(() -> query("MERGE INTO " + tableName + " USING (VALUES 1) t(x) ON false" +
" WHEN NOT MATCHED THEN INSERT VALUES ('str4', 4)"))
.hasMessageStartingWith("Location does not have parent: ");
return;
}
assertUpdate("MERGE INTO " + tableName + " USING (VALUES 1) t(x) ON false" +
" 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)");
Expand Down Expand Up @@ -226,12 +220,6 @@ public void testOptimizeWithProvidedTableLocation(boolean partitioned, LocationP
"WITH (" + locationQueryPart + partitionQueryPart + ")");
try (UncheckedCloseable ignored = onClose("DROP TABLE " + tableName)) {
// create multiple data files, INSERT with multiple values would create only one file (if not partitioned)
if (locationPattern == TWO_TRAILING_SLASHES && getClass().getName().contains(".deltalake.")) {
// TODO (https://github.com/trinodb/trino/issues/17966): writes fail when Delta table is declared with location ending with two slashes
assertThatThrownBy(() -> query("INSERT INTO " + tableName + " VALUES (1, 'one')"))
.hasMessageStartingWith("Location does not have parent: ");
return;
}
assertUpdate("INSERT INTO " + tableName + " VALUES (1, 'one')", 1);
assertUpdate("INSERT INTO " + tableName + " VALUES (2, 'a//double_slash')", 1);
assertUpdate("INSERT INTO " + tableName + " VALUES (3, 'a%percent')", 1);
Expand Down

0 comments on commit 05b9fc4

Please sign in to comment.