Skip to content

Do not create temp staging directory for CREATE TABLE without data #5802

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
laurachenyu wants to merge 1 commit into from
Closed

Do not create temp staging directory for CREATE TABLE without data #5802

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions presto-hive-hadoop2/src/test/java/io/prestosql/plugin/hive/TestHiveAlluxioMetastore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -309,4 +309,10 @@ public void testViewCreation()
{
// Alluxio metastore does not support create operations
}

@Override
public void testCreateEmptyTableStaging()
{
// Alluxio metastore does not support create operations
}
}
4 changes: 2 additions & 2 deletions presto-hive/src/main/java/io/prestosql/plugin/hive/HiveLocationService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ public HiveLocationService(HdfsEnvironment hdfsEnvironment)
}

@Override
public LocationHandle forNewTable(SemiTransactionalHiveMetastore metastore, ConnectorSession session, String schemaName, String tableName, Optional<Path> externalLocation)
public LocationHandle forNewTable(SemiTransactionalHiveMetastore metastore, ConnectorSession session, String schemaName, String tableName, Optional<Path> externalLocation, boolean withData)
Comment thread
sopel39 marked this conversation as resolved.
Outdated
{
HdfsContext context = new HdfsContext(session, schemaName, tableName);
Path targetPath = externalLocation.orElseGet(() -> getTableDefaultLocation(context, metastore, hdfsEnvironment, schemaName, tableName));
Expand All @@ -64,7 +64,7 @@ public LocationHandle forNewTable(SemiTransactionalHiveMetastore metastore, Conn
}

// TODO detect when existing table's location is a on a different file system than the temporary directory
if (shouldUseTemporaryDirectory(session, context, targetPath, externalLocation)) {
if (withData && shouldUseTemporaryDirectory(session, context, targetPath, externalLocation)) {
Comment thread
sopel39 marked this conversation as resolved.
Outdated
Path writePath = createTemporaryPath(session, context, hdfsEnvironment, targetPath);
return new LocationHandle(targetPath, writePath, false, STAGE_AND_MOVE_TO_TARGET_DIRECTORY);
}
Expand Down
4 changes: 2 additions & 2 deletions presto-hive/src/main/java/io/prestosql/plugin/hive/HiveMetadata.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -877,7 +877,7 @@ public void createTable(ConnectorSession session, ConnectorTableMetadata tableMe
}
else {
external = false;
LocationHandle locationHandle = locationService.forNewTable(metastore, session, schemaName, tableName, Optional.empty());
LocationHandle locationHandle = locationService.forNewTable(metastore, session, schemaName, tableName, Optional.empty(), false);
targetPath = locationService.getQueryWriteInfo(locationHandle).getTargetPath();
}

Expand Down Expand Up @@ -1325,7 +1325,7 @@ public HiveOutputTableHandle beginCreateTable(ConnectorSession session, Connecto
.collect(toList());
checkPartitionTypesSupported(partitionColumns);

LocationHandle locationHandle = locationService.forNewTable(metastore, session, schemaName, tableName, externalLocation);
LocationHandle locationHandle = locationService.forNewTable(metastore, session, schemaName, tableName, externalLocation, true);

boolean transactional = isTransactional(tableMetadata.getProperties()).orElse(false);
AcidTransaction transaction = transactional ? forCreateTable() : NO_ACID_TRANSACTION;
Expand Down
2 changes: 1 addition & 1 deletion presto-hive/src/main/java/io/prestosql/plugin/hive/LocationService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@

public interface LocationService
{
LocationHandle forNewTable(SemiTransactionalHiveMetastore metastore, ConnectorSession session, String schemaName, String tableName, Optional<Path> externalLocation);
LocationHandle forNewTable(SemiTransactionalHiveMetastore metastore, ConnectorSession session, String schemaName, String tableName, Optional<Path> externalLocation, boolean withData);

LocationHandle forExistingTable(SemiTransactionalHiveMetastore metastore, ConnectorSession session, Table table);

Expand Down
60 changes: 57 additions & 3 deletions presto-hive/src/test/java/io/prestosql/plugin/hive/AbstractTestHive.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,7 @@
import static io.prestosql.plugin.hive.util.HiveUtil.toPartitionValues;
import static io.prestosql.plugin.hive.util.HiveWriteUtils.createDirectory;
import static io.prestosql.plugin.hive.util.HiveWriteUtils.getTableDefaultLocation;
import static io.prestosql.plugin.hive.util.HiveWriteUtils.pathExists;
import static io.prestosql.spi.StandardErrorCode.NOT_SUPPORTED;
import static io.prestosql.spi.StandardErrorCode.TRANSACTION_CONFLICT;
import static io.prestosql.spi.connector.ConnectorSplitManager.SplitSchedulingStrategy.UNGROUPED_SCHEDULING;
Expand Down Expand Up @@ -2207,7 +2208,7 @@ public void testTableCreationIgnoreExisting()
try {
try (Transaction transaction = newTransaction()) {
LocationService locationService = getLocationService();
LocationHandle locationHandle = locationService.forNewTable(transaction.getMetastore(), session, schemaName, tableName, Optional.empty());
LocationHandle locationHandle = locationService.forNewTable(transaction.getMetastore(), session, schemaName, tableName, Optional.empty(), false);
targetPath = locationService.getQueryWriteInfo(locationHandle).getTargetPath();
Table table = createSimpleTable(schemaTableName, columns, session, targetPath, "q1");
transaction.getMetastore()
Expand Down Expand Up @@ -2539,6 +2540,59 @@ public void testEmptyTableCreation()
}
}

@Test
public void testCreateEmptyTableStaging()
{
for (HiveStorageFormat storageFormat : createTableFormats) {
SchemaTableName temporaryCreateEmptyTable = temporaryTable("create_empty_staging");
try {
List<Column> columns = ImmutableList.of(new Column("test", HIVE_STRING, Optional.empty()));
try (Transaction transaction = newTransaction()) {
ConnectorSession session = newSession();
String tableOwner = session.getUser();
String schemaName = temporaryCreateEmptyTable.getSchemaName();
String tableName = temporaryCreateEmptyTable.getTableName();
LocationService locationService = getLocationService();
LocationHandle locationHandle = locationService.forNewTable(transaction.getMetastore(),
Copy link
Copy Markdown
Member

@sopel39 sopel39 Dec 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

each arg in newline

session,
schemaName,
tableName,
Optional.empty(),
false);
Path writePath = locationService.getQueryWriteInfo(locationHandle).getWritePath();
Path targetPath = locationService.getQueryWriteInfo(locationHandle).getTargetPath();
Table.Builder tableBuilder = Table.builder()
.setDatabaseName(schemaName)
.setTableName(tableName)
.setOwner(tableOwner)
.setTableType(TableType.MANAGED_TABLE.name())
.setParameters(ImmutableMap.of(
PRESTO_VERSION_NAME, TEST_SERVER_VERSION,
PRESTO_QUERY_ID_NAME, session.getQueryId()))
.setDataColumns(columns)
.setPartitionColumns(ImmutableList.of());
tableBuilder.getStorageBuilder()
.setLocation(targetPath.toString())
.setStorageFormat(StorageFormat.create(storageFormat.getSerDe(), storageFormat.getInputFormat(), storageFormat.getOutputFormat()))
.setBucketProperty(Optional.empty())
.setSerdeParameters(ImmutableMap.of());
PrincipalPrivileges principalPrivileges = testingPrincipalPrivilege(tableOwner, session.getUser());
transaction.getMetastore().createTable(session, tableBuilder.build(), principalPrivileges, Optional.empty(), true, EMPTY_TABLE_STATISTICS);
transaction.commit();

HdfsContext context = new HdfsContext(session, schemaName, tableName);
if (locationHandle.getWriteMode() == STAGE_AND_MOVE_TO_TARGET_DIRECTORY) {
assertFalse(pathExists(context, hdfsEnvironment, writePath),
Copy link
Copy Markdown
Member

@sopel39 sopel39 Dec 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

io.prestosql.plugin.hive.util.HiveWriteUtils#createTemporaryPath uses randomUUID, so different temp directory will be used in transaction.getMetastore().createTable than writePath.

What you should do instead is to set some random staging directory (via temporary_staging_directory_path session property) and check that no dir was created there after createTable

"temp staging directory " + writePath.toString() + " exists");
}
}
}
finally {
dropTable(temporaryCreateEmptyTable);
}
}
}

@Test
public void testViewCreation()
{
Expand Down Expand Up @@ -2783,7 +2837,7 @@ public void testIllegalStorageFormatDuringTableScan()
String tableOwner = session.getUser();
String schemaName = schemaTableName.getSchemaName();
String tableName = schemaTableName.getTableName();
LocationHandle locationHandle = locationService.forNewTable(transaction.getMetastore(), session, schemaName, tableName, Optional.empty());
LocationHandle locationHandle = locationService.forNewTable(transaction.getMetastore(), session, schemaName, tableName, Optional.empty(), false);
Path targetPath = locationService.getQueryWriteInfo(locationHandle).getTargetPath();
//create table whose storage format is null
Table.Builder tableBuilder = Table.builder()
Expand Down Expand Up @@ -4730,7 +4784,7 @@ private void createEmptyTable(SchemaTableName schemaTableName, HiveStorageFormat
String tableName = schemaTableName.getTableName();

LocationService locationService = getLocationService();
LocationHandle locationHandle = locationService.forNewTable(transaction.getMetastore(), session, schemaName, tableName, Optional.empty());
LocationHandle locationHandle = locationService.forNewTable(transaction.getMetastore(), session, schemaName, tableName, Optional.empty(), false);
targetPath = locationService.getQueryWriteInfo(locationHandle).getTargetPath();

Table.Builder tableBuilder = Table.builder()
Expand Down