[GitHub] [iceberg] marton-bod commented on a change in pull request #1917: Hive: Convert the CREATE TABLE ... PARTITIONED BY to Iceberg identity partitions

[GitBox <gi...@apache.org>]

marton-bod commented on a change in pull request #1917:
URL: https://github.com/apache/iceberg/pull/1917#discussion_r541569540



##########
File path: mr/src/test/java/org/apache/iceberg/mr/hive/TestHiveIcebergStorageHandler.java
##########
@@ -666,45 +666,52 @@ public void testCreateTableWithColumnSpecification() throws IOException {
   }
 
   @Test
-  public void testCreateTableWithColumnSpecificationPartitioned() {
+  public void testCreateTableWithColumnSpecificationPartitioned() throws IOException {
+    TableIdentifier identifier = TableIdentifier.of("default", "customers");
+    runPartitionedWriteTest(identifier, createSQLForPartitionedTableBySchema(testTables, identifier));
+  }
+
+  @Test
+  public void testCreatePartitionedTableByProperty() throws IOException {
+    TableIdentifier identifier = TableIdentifier.of("default", "customers");
+    runPartitionedWriteTest(identifier, createSQLForPartitionedTableByProperty(testTables, identifier));
+  }
+
+  @Test
+  public void testCreatePartitionedTableWithPropertiesAndWithColumnSpecification() {
+    PartitionSpec spec = PartitionSpec.builderFor(CUSTOMER_SCHEMA).identity("last_name").build();
+
     AssertHelpers.assertThrows("should throw exception", IllegalArgumentException.class,
-        "currently not supported", () -> {
+        "Provide only one of the following", () -> {
           shell.executeStatement("CREATE EXTERNAL TABLE customers (customer_id BIGINT) " +
               "PARTITIONED BY (first_name STRING) " +
               "STORED BY 'org.apache.iceberg.mr.hive.HiveIcebergStorageHandler' " +
-              testTables.locationForCreateTableSQL(TableIdentifier.of("default", "customers")));
+              testTables.locationForCreateTableSQL(TableIdentifier.of("default", "customers")) +
+              " TBLPROPERTIES ('" + InputFormatConfig.PARTITION_SPEC + "'='" +
+              PartitionSpecParser.toJson(spec) + "')");
         }
     );
   }
 
   @Test
-  public void testCreatePartitionedTable() throws IOException {
+  public void testCreateTableWithColumnSpecificationMultilevelPartitioned() {

Review comment:
       Can we make `runPartitionedWriteTest` generic enough to reuse it here too? Would be great to have an execution test for multi-level partition as well.

##########
File path: mr/src/main/java/org/apache/iceberg/mr/hive/HiveIcebergMetaHook.java
##########
@@ -197,20 +205,32 @@ private static Schema schema(Properties properties, org.apache.hadoop.hive.metas
     if (properties.getProperty(InputFormatConfig.TABLE_SCHEMA) != null) {
       return SchemaParser.fromJson(properties.getProperty(InputFormatConfig.TABLE_SCHEMA));
     } else {
-      return HiveSchemaUtil.convert(hmsTable.getSd().getCols());
+      if (hmsTable.isSetPartitionKeys() && hmsTable.getPartitionKeys().size() != 0) {

Review comment:
       nit: can use `!hmsTable.getPartitionKeys().isEmpty()`




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org