hive-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "ASF GitHub Bot (Jira)" <j...@apache.org>
Subject [jira] [Work logged] (HIVE-22826) ALTER TABLE RENAME COLUMN doesn't update list of bucketed column names
Date Fri, 02 Oct 2020 11:10:00 GMT

     [ https://issues.apache.org/jira/browse/HIVE-22826?focusedWorklogId=493896&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-493896
]

ASF GitHub Bot logged work on HIVE-22826:
-----------------------------------------

                Author: ASF GitHub Bot
            Created on: 02/Oct/20 11:09
            Start Date: 02/Oct/20 11:09
    Worklog Time Spent: 10m 
      Work Description: sankarh commented on a change in pull request #1528:
URL: https://github.com/apache/hive/pull/1528#discussion_r498651478



##########
File path: ql/src/java/org/apache/hadoop/hive/ql/ddl/table/column/change/AlterTableChangeColumnOperation.java
##########
@@ -72,6 +74,10 @@ protected void doAlteration(Table table, Partition partition) throws HiveExcepti
         if (desc.getNewColumnComment() != null) {
           oldColumn.setComment(desc.getNewColumnComment());
         }
+        if (CollectionUtils.isNotEmpty(sd.getBucketCols()) && sd.getBucketCols().contains(oldColumnName))
{
+          sd.getBucketCols().remove(oldColumnName);
+          sd.getBucketCols().add(desc.getNewColumnName());

Review comment:
       Should we store it in lower-case?

##########
File path: ql/src/java/org/apache/hadoop/hive/ql/ddl/table/column/change/AlterTableChangeColumnOperation.java
##########
@@ -72,6 +74,10 @@ protected void doAlteration(Table table, Partition partition) throws HiveExcepti
         if (desc.getNewColumnComment() != null) {
           oldColumn.setComment(desc.getNewColumnComment());
         }
+        if (CollectionUtils.isNotEmpty(sd.getBucketCols()) && sd.getBucketCols().contains(oldColumnName))
{

Review comment:
       Is oldColumnName case in-sensitive?

##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java
##########
@@ -130,6 +130,11 @@ public void alterTable(RawStore msdb, Warehouse wh, String catName, String
dbnam
       throw new InvalidOperationException("Invalid column " + validate);
     }
 
+    // Validate bucketedColumns in new table
+    if (!MetaStoreServerUtils.validateBucketColumns(newt.getSd())) {
+      throw new InvalidOperationException("Bucket column doesn't match with any table columns");

Review comment:
       Useful to add an error log with the column name which is missing from table columns
list.

##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreServerUtils.java
##########
@@ -1554,4 +1555,32 @@ public static Partition createMetaPartitionObject(Table tbl, Map<String,
String>
     }
     return tpart;
   }
+
+  /**
+   * Validate bucket columns should belong to table columns.
+   * @param sd StorageDescriptor of given table
+   * @return true if bucket columns are empty or belong to table columns else false
+   */
+  public static boolean validateBucketColumns(StorageDescriptor sd) {
+    List<String> columnNames = getColumnNames(sd.getCols());
+    if(CollectionUtils.isNotEmpty(sd.getBucketCols()) &&  CollectionUtils.isNotEmpty(columnNames)){
+      return columnNames.containsAll(sd.getBucketCols().stream().map(String::toLowerCase).collect(Collectors.toList()));
+    } else if (CollectionUtils.isNotEmpty(sd.getBucketCols()) &&  CollectionUtils.isEmpty(columnNames))
{
+      return false;
+    } else {
+      return true;
+    }
+  }
+
+  /**
+   * Generate column name list  from the fieldSchema list
+   * @param cols fieldSchema list
+   * @return column name list
+   */
+  public static List<String> getColumnNames(List<FieldSchema> cols) {
+    if (CollectionUtils.isNotEmpty(cols)) {
+      return cols.stream().map(FieldSchema::getName).collect(Collectors.toList());

Review comment:
       Will cols always have names in lower case?

##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreServerUtils.java
##########
@@ -1554,4 +1555,32 @@ public static Partition createMetaPartitionObject(Table tbl, Map<String,
String>
     }
     return tpart;
   }
+
+  /**
+   * Validate bucket columns should belong to table columns.
+   * @param sd StorageDescriptor of given table
+   * @return true if bucket columns are empty or belong to table columns else false
+   */
+  public static boolean validateBucketColumns(StorageDescriptor sd) {
+    List<String> columnNames = getColumnNames(sd.getCols());
+    if(CollectionUtils.isNotEmpty(sd.getBucketCols()) &&  CollectionUtils.isNotEmpty(columnNames)){

Review comment:
       nit: Add space before "("

##########
File path: ql/src/test/queries/clientpositive/alter_numbuckets_partitioned_table_h23.q
##########
@@ -52,6 +52,12 @@ alter table tst1_n1 clustered by (value) into 12 buckets;
 
 describe formatted tst1_n1;
 
+-- Test changing name of bucket column
+
+alter table tst1_n1 change key keys string;
+
+describe formatted tst1_n1;

Review comment:
       Also check the output of "show create table" command.

##########
File path: ql/src/test/queries/clientpositive/alter_bucketedtable_change_column.q
##########
@@ -0,0 +1,10 @@
+--! qt:dataset:src
+create table alter_bucket_change_col_t1(key string, value string) partitioned by (ds string)
clustered by (key) into 10 buckets;
+
+describe formatted alter_bucket_change_col_t1;
+
+-- Test changing name of bucket column
+
+alter table alter_bucket_change_col_t1 change key keys string;

Review comment:
       Add test for columns names with mix of upper and lower case letters.

##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreServerUtils.java
##########
@@ -1554,4 +1555,32 @@ public static Partition createMetaPartitionObject(Table tbl, Map<String,
String>
     }
     return tpart;
   }
+
+  /**
+   * Validate bucket columns should belong to table columns.
+   * @param sd StorageDescriptor of given table
+   * @return true if bucket columns are empty or belong to table columns else false
+   */
+  public static boolean validateBucketColumns(StorageDescriptor sd) {
+    List<String> columnNames = getColumnNames(sd.getCols());

Review comment:
       Check if CollectionUtils.isEmpty(sd.getBucketCols()) case even before getting table
columns list and return true. It will avoid repeated checks below and also avoids unnecessary
computes for non-bucketed tables.




----------------------------------------------------------------
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


Issue Time Tracking
-------------------

    Worklog Id:     (was: 493896)
    Time Spent: 1h 10m  (was: 1h)

>  ALTER TABLE RENAME COLUMN doesn't update list of bucketed column names
> -----------------------------------------------------------------------
>
>                 Key: HIVE-22826
>                 URL: https://issues.apache.org/jira/browse/HIVE-22826
>             Project: Hive
>          Issue Type: Bug
>          Components: Query Planning
>    Affects Versions: 4.0.0
>            Reporter: Karen Coppage
>            Assignee: Ashish Sharma
>            Priority: Major
>              Labels: pull-request-available
>         Attachments: unitTest.patch
>
>          Time Spent: 1h 10m
>  Remaining Estimate: 0h
>
> Compaction for tables where a bucketed column has been renamed fails since the list of
bucketed columns in the StorageDescriptor doesn't get updated when the column is renamed,
therefore we can't recreate the table correctly during compaction.
> Attached a unit test that fails.
> NO PRECOMMIT TESTS



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

Mime
View raw message