Skip to content

Commit

Permalink
Fix setting iceberg table location on creation for REST/NESSIE catalog
Browse files Browse the repository at this point in the history
  • Loading branch information
@hantangwangd @tdcmeehan
hantangwangd authored and tdcmeehan committed Dec 11, 2024
1 parent 004ee32 commit d4e878b
Show file tree
Hide file tree
Showing 4 changed files with 101 additions and 34 deletions.
24 changes: 19 additions & 5 deletions presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergNativeMetadata.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@
import static com.facebook.presto.iceberg.IcebergSessionProperties.getCompressionCodec;
import static com.facebook.presto.iceberg.IcebergTableProperties.getFileFormat;
import static com.facebook.presto.iceberg.IcebergTableProperties.getPartitioning;
import static com.facebook.presto.iceberg.IcebergTableProperties.getTableLocation;
import static com.facebook.presto.iceberg.IcebergTableType.DATA;
import static com.facebook.presto.iceberg.IcebergUtil.VIEW_OWNER;
import static com.facebook.presto.iceberg.IcebergUtil.createIcebergViewProperties;
Expand All @@ -76,6 +77,7 @@
import static com.facebook.presto.iceberg.util.IcebergPrestoModelConverters.toPrestoSchemaTableName;
import static com.facebook.presto.spi.StandardErrorCode.NOT_SUPPORTED;
import static com.facebook.presto.spi.StandardErrorCode.SCHEMA_NOT_EMPTY;
import static com.google.common.base.Strings.isNullOrEmpty;
import static com.google.common.base.Verify.verify;
import static java.lang.String.format;
import static java.util.Objects.requireNonNull;
Expand Down Expand Up @@ -313,11 +315,23 @@ public ConnectorOutputTableHandle beginCreateTable(ConnectorSession session, Con
FileFormat fileFormat = getFileFormat(tableMetadata.getProperties());

try {
transaction = catalogFactory.getCatalog(session).newCreateTableTransaction(
toIcebergTableIdentifier(schemaTableName, catalogFactory.isNestedNamespaceEnabled()),
schema,
partitionSpec,
populateTableProperties(tableMetadata, fileFormat, session));
TableIdentifier tableIdentifier = toIcebergTableIdentifier(schemaTableName, catalogFactory.isNestedNamespaceEnabled());
String targetPath = getTableLocation(tableMetadata.getProperties());
if (!isNullOrEmpty(targetPath)) {
transaction = catalogFactory.getCatalog(session).newCreateTableTransaction(
tableIdentifier,
schema,
partitionSpec,
targetPath,
populateTableProperties(tableMetadata, fileFormat, session));
}
else {
transaction = catalogFactory.getCatalog(session).newCreateTableTransaction(
tableIdentifier,
schema,
partitionSpec,
populateTableProperties(tableMetadata, fileFormat, session));
}
}
catch (AlreadyExistsException e) {
throw new TableAlreadyExistsException(schemaTableName);
Expand Down
78 changes: 49 additions & 29 deletions ...to-iceberg/src/test/java/com/facebook/presto/iceberg/IcebergDistributedSmokeTestBase.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
import static com.facebook.presto.SystemSessionProperties.LEGACY_TIMESTAMP;
import static com.facebook.presto.common.type.TimeZoneKey.UTC_KEY;
import static com.facebook.presto.common.type.VarcharType.VARCHAR;
import static com.facebook.presto.iceberg.CatalogType.HADOOP;
import static com.facebook.presto.iceberg.IcebergQueryRunner.ICEBERG_CATALOG;
import static com.facebook.presto.iceberg.IcebergQueryRunner.getIcebergDataDirectoryPath;
import static com.facebook.presto.iceberg.IcebergUtil.MIN_FORMAT_VERSION_FOR_DELETE;
Expand Down Expand Up @@ -753,35 +754,54 @@ private void testCreateTableLike()
")", getLocation(schemaName, "test_create_table_like_copy2")));
dropTable(session, "test_create_table_like_copy2");

assertUpdate(session, "CREATE TABLE test_create_table_like_copy3 (LIKE test_create_table_like_original INCLUDING PROPERTIES)");
assertEquals(getTablePropertiesString("test_create_table_like_copy3"), format("WITH (\n" +
" delete_mode = 'merge-on-read',\n" +
" format = 'PARQUET',\n" +
" format_version = '2',\n" +
" location = '%s',\n" +
" metadata_delete_after_commit = false,\n" +
" metadata_previous_versions_max = 100,\n" +
" metrics_max_inferred_column = 100,\n" +
" partitioning = ARRAY['adate']\n" +
")", catalogType.equals(CatalogType.HIVE) ?
getLocation(schemaName, "test_create_table_like_original") :
getLocation(schemaName, "test_create_table_like_copy3")));
dropTable(session, "test_create_table_like_copy3");

assertUpdate(session, "CREATE TABLE test_create_table_like_copy4 (LIKE test_create_table_like_original INCLUDING PROPERTIES) WITH (format = 'ORC')");
assertEquals(getTablePropertiesString("test_create_table_like_copy4"), format("WITH (\n" +
" delete_mode = 'merge-on-read',\n" +
" format = 'ORC',\n" +
" format_version = '2',\n" +
" location = '%s',\n" +
" metadata_delete_after_commit = false,\n" +
" metadata_previous_versions_max = 100,\n" +
" metrics_max_inferred_column = 100,\n" +
" partitioning = ARRAY['adate']\n" +
")", catalogType.equals(CatalogType.HIVE) ?
getLocation(schemaName, "test_create_table_like_original") :
getLocation(schemaName, "test_create_table_like_copy4")));
dropTable(session, "test_create_table_like_copy4");
if (!catalogType.equals(HADOOP)) {
assertUpdate(session, "CREATE TABLE test_create_table_like_copy3 (LIKE test_create_table_like_original INCLUDING PROPERTIES)");
assertEquals(getTablePropertiesString("test_create_table_like_copy3"), format("WITH (\n" +
" delete_mode = 'merge-on-read',\n" +
" format = 'PARQUET',\n" +
" format_version = '2',\n" +
" location = '%s',\n" +
" metadata_delete_after_commit = false,\n" +
" metadata_previous_versions_max = 100,\n" +
" metrics_max_inferred_column = 100,\n" +
" partitioning = ARRAY['adate']\n" +
")",
getLocation(schemaName, "test_create_table_like_original")));
dropTable(session, "test_create_table_like_copy3");

assertUpdate(session, "CREATE TABLE test_create_table_like_copy4 (LIKE test_create_table_like_original INCLUDING PROPERTIES) WITH (format = 'ORC')");
assertEquals(getTablePropertiesString("test_create_table_like_copy4"), format("WITH (\n" +
" delete_mode = 'merge-on-read',\n" +
" format = 'ORC',\n" +
" format_version = '2',\n" +
" location = '%s',\n" +
" metadata_delete_after_commit = false,\n" +
" metadata_previous_versions_max = 100,\n" +
" metrics_max_inferred_column = 100,\n" +
" partitioning = ARRAY['adate']\n" +
")",
getLocation(schemaName, "test_create_table_like_original")));
dropTable(session, "test_create_table_like_copy4");
}
else {
assertUpdate(session, "CREATE TABLE test_create_table_like_copy5 (LIKE test_create_table_like_original INCLUDING PROPERTIES)" +
" WITH (location = '', format = 'ORC')");
assertEquals(getTablePropertiesString("test_create_table_like_copy5"), format("WITH (\n" +
" delete_mode = 'merge-on-read',\n" +
" format = 'ORC',\n" +
" format_version = '2',\n" +
" location = '%s',\n" +
" metadata_delete_after_commit = false,\n" +
" metadata_previous_versions_max = 100,\n" +
" metrics_max_inferred_column = 100,\n" +
" partitioning = ARRAY['adate']\n" +
")",
getLocation(schemaName, "test_create_table_like_copy5")));
dropTable(session, "test_create_table_like_copy5");

assertQueryFails(session, "CREATE TABLE test_create_table_like_copy6 (LIKE test_create_table_like_original INCLUDING PROPERTIES)",
"Cannot set a custom location for a path-based table.*");
}

dropTable(session, "test_create_table_like_original");
}
Expand Down
20 changes: 20 additions & 0 deletions presto-iceberg/src/test/java/com/facebook/presto/iceberg/IcebergDistributedTestBase.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@
import java.io.File;
import java.io.IOException;
import java.lang.reflect.Field;
import java.net.URI;
import java.nio.ByteBuffer;
import java.nio.file.Path;
import java.time.LocalDateTime;
Expand Down Expand Up @@ -134,6 +135,7 @@
import static com.facebook.presto.testing.TestingConnectorSession.SESSION;
import static com.facebook.presto.testing.assertions.Assert.assertEquals;
import static com.facebook.presto.tests.sql.TestTable.randomTableSuffix;
import static com.google.common.io.Files.createTempDir;
import static java.lang.String.format;
import static java.util.Objects.requireNonNull;
import static java.util.UUID.randomUUID;
Expand Down Expand Up @@ -582,6 +584,24 @@ private void testPartitionedByTimestampTypeForFormat(Session session, FileFormat
}
}

@Test
public void testCreateTableWithCustomLocation()
{
String tableName = "test_table_with_custom_location";
URI tableTargetURI = createTempDir().toURI();
try {
assertQuerySucceeds(format("create table %s (a int, b varchar)" +
" with (location = '%s')", tableName, tableTargetURI.toString()));
assertUpdate(format("insert into %s values(1, '1001'), (2, '1002')", tableName), 2);
assertQuery("select * from " + tableName, "values(1, '1001'), (2, '1002')");
TableMetadata tableMetadata = ((BaseTable) loadTable(tableName)).operations().current();
assertEquals(URI.create(tableMetadata.location() + File.separator), tableTargetURI);
}
finally {
assertUpdate("drop table if exists " + tableName);
}
}

@Test
public void testPartitionedByTimeType()
{
Expand Down
13 changes: 13 additions & 0 deletions ...ceberg/src/test/java/com/facebook/presto/iceberg/hadoop/TestIcebergDistributedHadoop.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,11 @@
import com.facebook.presto.iceberg.IcebergDistributedTestBase;
import org.testng.annotations.Test;

import java.net.URI;

import static com.facebook.presto.iceberg.CatalogType.HADOOP;
import static com.google.common.io.Files.createTempDir;
import static java.lang.String.format;

@Test
public class TestIcebergDistributedHadoop
Expand All @@ -26,4 +30,13 @@ public TestIcebergDistributedHadoop()
{
super(HADOOP);
}

@Override
public void testCreateTableWithCustomLocation()
{
String tableName = "test_hadoop_table_with_custom_location";
URI tableTargetURI = createTempDir().toURI();
assertQueryFails(format("create table %s (a int, b varchar)" + " with (location = '%s')", tableName, tableTargetURI.toString()),
"Cannot set a custom location for a path-based table.*");
}
}

0 comments on commit d4e878b

Please sign in to comment.