Skip to content
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

Fix creating non-bucketed empty partition #12119

Closed
wants to merge 1 commit into from
Closed

Fix creating non-bucketed empty partition #12119

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
7 changes: 7 additions & 0 deletions presto-hive/src/main/java/com/facebook/presto/hive/HiveMetadata.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1220,6 +1220,13 @@ public Optional<ConnectorOutputMetadata> finishInsert(ConnectorSession session,
Map<List<String>, ComputedStatistics> partitionComputedStatistics = createComputedStatisticsToPartitionMap(computedStatistics, partitionedBy, columnTypes);

for (PartitionUpdate partitionUpdate : partitionUpdates) {
if (partitionUpdate.getFileNames().size() == 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Use isEmpty()

HiveWriteUtils.createDirectory(
Copy link
Contributor

Choose a reason for hiding this comment

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

Static import

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think creating the directory here is there correct place. We should be doing it in SemiTransactionalHiveMetastore as part of the commit. We probably also need to handle it in the rollback cleaup. @haozhun should take a look

new HdfsContext(session, table.get().getDatabaseName(), table.get().getTableName()),
hdfsEnvironment,
partitionUpdate.getWritePath());
}

if (partitionUpdate.getName().isEmpty()) {
// insert into unpartitioned table
metastore.finishInsertIntoExistingTable(
Expand Down
24 changes: 24 additions & 0 deletions presto-hive/src/test/java/com/facebook/presto/hive/TestHiveIntegrationSmokeTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -937,6 +937,30 @@ public void testCastNullToColumnTypes()
assertUpdate("DROP TABLE " + tableName);
}

@Test
public void testCreateEmptyPartition()
{
String tableName = "empty_partition_table";
assertUpdate(format("" +
"CREATE TABLE %s " +
"WITH ( " +
" FORMAT = 'ORC', " +
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: lowercase the name format and use consistent indentation with the other property (two spaces)

" partitioned_by = ARRAY['p_varchar'] " +
") " +
"AS " +
Copy link
Contributor

Choose a reason for hiding this comment

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

Creating an empty table should be sufficient. We should be able to create a new table and then add an empty partition. Is there a reason why another partition was needed, or something you wanted test?

"SELECT c_bigint, p_varchar " +
"FROM ( " +
" VALUES " +
" (BIGINT '7', 'longlonglong')" +
") AS x (c_bigint, p_varchar)", tableName), 1);
assertQuery(format("SELECT count(*) FROM \"%s$partitions\"", tableName), "SELECT 1");

// create an empty partition
assertUpdate(format("CALL system.create_empty_partition('%s', '%s', ARRAY['p_varchar'], ARRAY['%s'])", TPCH_SCHEMA, tableName, "empty"));
assertQuery(format("SELECT count(*) FROM \"%s$partitions\"", tableName), "SELECT 2");
assertUpdate("DROP TABLE " + tableName);
}

@Test
public void testCreateEmptyBucketedPartition()
{
Expand Down