Repository: prestodb/presto
Issue: Validate column order when creating Hive tables

Partition columns for Hive tables are always at the end because Hive works that way. When creating a table, the creation should fail if the columns are not in the final order (having `CREATE TABLE` silently rearrange columns is quite unexpected).

We should use a new error code for this: `HIVE_COLUMN_ORDER_MISMATCH`

This should fail because the partition column is listed first, but will be at the end in the final table:

```
CREATE TABLE test (
  ds varchar,
  x bigint)
WITH (partitioned_by = ARRAY['ds']);
```

This should fail because the partition columns are listed in a different order than in the partition specification:

```
CREATE TABLE test (
  x bigint,
  type varchar,
  ds varchar)
WITH (partitioned_by = ARRAY['ds', 'type']);
```


Changed files:
presto-hive/src/test/java/com/facebook/presto/hive/TestHiveIntegrationSmokeTest.java
presto-hive/src/main/java/com/facebook/presto/hive/HiveMetadata.java
presto-hive/src/main/java/com/facebook/presto/hive/HiveErrorCode.java

diff --git a/presto-hive/src/main/java/com/facebook/presto/hive/HiveErrorCode.java b/presto-hive/src/main/java/com/facebook/presto/hive/HiveErrorCode.java
index c902673fab..5da341a3cd 100644
--- a/presto-hive/src/main/java/com/facebook/presto/hive/HiveErrorCode.java
+++ b/presto-hive/src/main/java/com/facebook/presto/hive/HiveErrorCode.java
@@ -42,7 +42,8 @@ public enum HiveErrorCode
     HIVE_UNSUPPORTED_FORMAT(0x0100_0013),
     HIVE_PARTITION_READ_ONLY(0x0100_00014),
     HIVE_TOO_MANY_OPEN_PARTITIONS(0x0000_0015),
-    HIVE_CONCURRENT_MODIFICATION_DETECTED(0x0000_0016);
+    HIVE_CONCURRENT_MODIFICATION_DETECTED(0x0000_0016),
+    HIVE_COLUMN_ORDER_MISMATCH(0x0000_0017);
 
     private final ErrorCode errorCode;
 
diff --git a/presto-hive/src/main/java/com/facebook/presto/hive/HiveMetadata.java b/presto-hive/src/main/java/com/facebook/presto/hive/HiveMetadata.java
index 05bd0aee5a..f4ab85d629 100644
--- a/presto-hive/src/main/java/com/facebook/presto/hive/HiveMetadata.java
+++ b/presto-hive/src/main/java/com/facebook/presto/hive/HiveMetadata.java
@@ -82,6 +82,7 @@ import java.util.function.Predicate;
 
 import static com.facebook.presto.hive.HiveColumnHandle.SAMPLE_WEIGHT_COLUMN_NAME;
 import static com.facebook.presto.hive.HiveColumnHandle.updateRowIdHandle;
+import static com.facebook.presto.hive.HiveErrorCode.HIVE_COLUMN_ORDER_MISMATCH;
 import static com.facebook.presto.hive.HiveErrorCode.HIVE_CONCURRENT_MODIFICATION_DETECTED;
 import static com.facebook.presto.hive.HiveErrorCode.HIVE_FILESYSTEM_ERROR;
 import static com.facebook.presto.hive.HiveErrorCode.HIVE_INVALID_METADATA;
@@ -1304,8 +1305,23 @@ public class HiveMetadata
         throw new PrestoException(HIVE_UNSUPPORTED_FORMAT, format("Output format %s with SerDe %s is not supported", outputFormat, serializationLib));
     }
 
+    private static void validateColumnOrdering(ConnectorTableMetadata tableMetadata)
+    {
+        List<String> partitionedBy = getPartitionedBy(tableMetadata.getProperties());
+
+        List<String> allColumns = tableMetadata.getColumns().stream()
+                .map(ColumnMetadata::getName)
+                .collect(toList());
+
+        if (!allColumns.subList(allColumns.size() - partitionedBy.size(), allColumns.size()).equals(partitionedBy)) {
+            throw new PrestoException(HIVE_COLUMN_ORDER_MISMATCH, "Partition keys must be the last columns in the table and in the same order as the table properties: " + partitionedBy);
+        }
+    }
+
     private static List<HiveColumnHandle> getColumnHandles(String connectorId, ConnectorTableMetadata tableMetadata, Set<String> partitionColumnNames)
     {
+        validateColumnOrdering(tableMetadata);
+
         ImmutableList.Builder<HiveColumnHandle> columnHandles = ImmutableList.builder();
         Set<String> foundPartitionColumns = new HashSet<>();
         int ordinal = 0;
diff --git a/presto-hive/src/test/java/com/facebook/presto/hive/TestHiveIntegrationSmokeTest.java b/presto-hive/src/test/java/com/facebook/presto/hive/TestHiveIntegrationSmokeTest.java
index 7dcb48d386..ff6017a4c4 100644
--- a/presto-hive/src/test/java/com/facebook/presto/hive/TestHiveIntegrationSmokeTest.java
+++ b/presto-hive/src/test/java/com/facebook/presto/hive/TestHiveIntegrationSmokeTest.java
@@ -144,13 +144,13 @@ public class TestHiveIntegrationSmokeTest
     {
         @Language("SQL") String createTable = "" +
                 "CREATE TABLE test_partitioned_table (" +
-                "  _partition_varchar VARCHAR" +
-                ", _partition_bigint BIGINT" +
-                ", _varchar VARCHAR" +
+                "  _varchar VARCHAR" +
                 ", _varbinary VARBINARY" +
                 ", _bigint BIGINT" +
                 ", _double DOUBLE" +
                 ", _boolean BOOLEAN" +
+                ", _partition_varchar VARCHAR" +
+                ", _partition_bigint BIGINT" +
                 ") " +
                 "WITH (" +
                 "format = '" + storageFormat + "', " +
@@ -244,7 +244,6 @@ public class TestHiveIntegrationSmokeTest
         List<HivePartition> partitions = getPartitions("test_create_partitioned_table_as");
         assertEquals(partitions.size(), 3);
 
-        // Hive will reorder the partition keys to the end
         assertQuery("SELECT * from test_create_partitioned_table_as", "SELECT orderkey, shippriority, orderstatus FROM orders");
 
         assertUpdate("DROP TABLE test_create_partitioned_table_as");
@@ -252,6 +251,27 @@ public class TestHiveIntegrationSmokeTest
         assertFalse(queryRunner.tableExists(getSession(), "test_create_partitioned_table_as"));
     }
 
+    @Test(expectedExceptions = RuntimeException.class, expectedExceptionsMessageRegExp = "Partition keys must be the last columns in the table and in the same order as the table properties.*")
+    public void testCreatePartitionedTableInvalidColumnOrdering()
+    {
+        assertUpdate("" +
+                "CREATE TABLE test_show_columns_partition_key\\n" +
+                "(grape bigint, apple varchar, orange bigint, pear varchar)\\n" +
+                "WITH (partitioned_by = ARRAY['apple'])");
+    }
+
+    @Test(expectedExceptions = RuntimeException.class, expectedExceptionsMessageRegExp = "Partition keys must be the last columns in the table and in the same order as the table properties.*")
+    public void testCreatePartitionedTableAsInvalidColumnOrdering()
+            throws Exception
+    {
+        assertUpdate("" +
+                "CREATE TABLE test_create_partitioned_table_as " +
+                "WITH (partitioned_by = ARRAY['SHIP_PRIORITY', 'ORDER_STATUS']) " +
+                "AS " +
+                "SELECT shippriority AS ship_priority, orderkey AS order_key, orderstatus AS order_status " +
+                "FROM tpch.tiny.orders");
+    }
+
     @Test
     public void insertTable()
             throws Exception
@@ -317,9 +337,9 @@ public class TestHiveIntegrationSmokeTest
         @Language("SQL") String createTable = "" +
                 "CREATE TABLE test_insert_partitioned_table " +
                 "(" +
-                "  ORDER_STATUS VARCHAR," +
+                "  ORDER_KEY BIGINT," +
                 "  SHIP_PRIORITY BIGINT," +
-                "  ORDER_KEY BIGINT" +
+                "  ORDER_STATUS VARCHAR" +
                 ") " +
                 "WITH (" +
                 "format = '" + storageFormat + "', " +
@@ -380,9 +400,9 @@ public class TestHiveIntegrationSmokeTest
         @Language("SQL") String createTable = "" +
                 "CREATE TABLE test_metadata_delete " +
                 "(" +
-                "  LINE_STATUS VARCHAR," +
+                "  ORDER_KEY BIGINT," +
                 "  LINE_NUMBER BIGINT," +
-                "  ORDER_KEY BIGINT" +
+                "  LINE_STATUS VARCHAR" +
                 ") " +
                 "WITH (" +
                 STORAGE_FORMAT_PROPERTY + " = '" + storageFormat + "', " +
@@ -391,7 +411,6 @@ public class TestHiveIntegrationSmokeTest
 
         assertUpdate(createTable);
 
-        // Hive will reorder the partition keys, so we must insert into the table assuming the partition keys have been moved to the end
         assertUpdate("" +
                         "INSERT INTO test_metadata_delete " +
                         "SELECT orderkey, linenumber, linestatus " +
@@ -459,10 +478,9 @@ public class TestHiveIntegrationSmokeTest
     {
         assertUpdate("" +
                 "CREATE TABLE test_show_columns_partition_key\\n" +
-                "(grape bigint, apple varchar, orange bigint, pear varchar)\\n" +
+                "(grape bigint, orange bigint, pear varchar, apple varchar)\\n" +
                 "WITH (partitioned_by = ARRAY['apple'])");
 
-        // partition keys go last and have a special comment
         MaterializedResult actual = computeActual("SHOW COLUMNS FROM test_show_columns_partition_key");
         MaterializedResult expected = resultBuilder(getSession(), VARCHAR, VARCHAR, VARCHAR)
                 .row("grape", "bigint", "")