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:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscr...@hive.apache.org
For additional commands, e-mail: gitbox-h...@hive.apache.org
