saihemanth-cloudera commented on code in PR #4684:
URL: https://github.com/apache/hive/pull/4684#discussion_r1336491353


##########
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/partition/alter/AlterTableAlterPartitionOperation.java:
##########
@@ -92,22 +91,23 @@ private void checkPartitionValues(Table tbl, int colIndex) 
throws HiveException
     Converter converter = ObjectInspectorConverters.getConverter(
         PrimitiveObjectInspectorFactory.javaStringObjectInspector, outputOI);
 
-    Set<Partition> partitions = context.getDb().getAllPartitionsOf(tbl);
-    for (Partition part : partitions) {
-      if 
(part.getName().equals(context.getConf().getVar(HiveConf.ConfVars.DEFAULTPARTITIONNAME)))
 {
-        continue;
-      }
-
+    List<String> partNames = context.getDb().getPartitionNames(tbl.getDbName(),

Review Comment:
   Also, will the listPartitions() call ignore 
'HiveConf.ConfVars.DEFAULTPARTITIONNAME' and return only the true partition 
names?



##########
ql/src/test/results/clientpositive/llap/alter_partition_coltype.q.out:
##########
@@ -453,6 +453,16 @@ POSTHOOK: Input: default@src
 POSTHOOK: Output: pt@alterdynamic_part_table
 POSTHOOK: Output: pt@alterdynamic_part_table@partcol1=1/partcol2=1
 POSTHOOK: Lineage: alterdynamic_part_table 
PARTITION(partcol1=1,partcol2=1).intcol EXPRESSION []
+PREHOOK: query: insert into table pt.alterdynamic_part_table 
partition(partcol1, partcol2) select '2', '2', NULL
+PREHOOK: type: QUERY
+PREHOOK: Input: _dummy_database@_dummy_table
+PREHOOK: Output: pt@alterdynamic_part_table
+POSTHOOK: query: insert into table pt.alterdynamic_part_table 
partition(partcol1, partcol2) select '2', '2', NULL
+POSTHOOK: type: QUERY
+POSTHOOK: Input: _dummy_database@_dummy_table
+POSTHOOK: Output: pt@alterdynamic_part_table
+POSTHOOK: Output: 
pt@alterdynamic_part_table@partcol1=2/partcol2=__HIVE_DEFAULT_PARTITION__

Review Comment:
   I see that listPartitionNames() is listing the default partition 
'__HIVE_DEFAULT_PARTITION__'. I think this is not what we expect right? We 
should ignore this value, right?



##########
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/partition/alter/AlterTableAlterPartitionOperation.java:
##########
@@ -92,22 +91,23 @@ private void checkPartitionValues(Table tbl, int colIndex) 
throws HiveException
     Converter converter = ObjectInspectorConverters.getConverter(
         PrimitiveObjectInspectorFactory.javaStringObjectInspector, outputOI);
 
-    Set<Partition> partitions = context.getDb().getAllPartitionsOf(tbl);
-    for (Partition part : partitions) {
-      if 
(part.getName().equals(context.getConf().getVar(HiveConf.ConfVars.DEFAULTPARTITIONNAME)))
 {
-        continue;
-      }
-
+    List<String> partNames = context.getDb().getPartitionNames(tbl.getDbName(),

Review Comment:
   Good work on eliminating List<Partitions> call because it is absolutely not 
required.
   One suggestion I have is, can we use listParititonNamesRequest() or 
listPartitionNames(catname, dbName, tableName, maxParts)?



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

To unsubscribe, e-mail: gitbox-unsubscr...@hive.apache.org

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

Reply via email to