zhangbutao commented on code in PR #5206:
URL: https://github.com/apache/hive/pull/5206#discussion_r1609589477
##########
ql/src/java/org/apache/hadoop/hive/ql/metadata/SessionHiveMetaStoreClient.java:
##########
@@ -1201,7 +1201,7 @@ public List<String> listPartitionNames(String catName,
String dbName, String tbl
public List<String> listPartitionNames(String catName, String dbName, String
tblName,
List<String> partVals, int maxParts) throws TException {
org.apache.hadoop.hive.metastore.api.Table table = getTempTable(dbName,
tblName);
- if (table == null) {
+ if (table == null || partVals == null) {
Review Comment:
I tried to understand you folk's talk:
@wecharyu thinks that some hms partition api support `null ` & `empty `,
such as `get_partitions_ps_with_auth`, but some hms partition api don't support
`null ` & `empty`. So, for temp table, check `null `& `empry` and always throw
exception in `PartitionTree ` in not inappropriate.
I think in this case, @wecharyu you need to make sure all the api's
behaviors which invoke the `PartitionTree ::getPartitionsByPartitionVals` are
unaffected. For example, `TempTable::getPartitionsByPartitionVals` is invoked
in four api, but i see that two of them are not changed after you change
`PartitionTree ::getPartitionsByPartitionVals`, is that safe?
@dengzhhu653 I am not very familair with temp table's ability.
`PartitionTree` is only used for temp table, if temp table has same ability
with common table, especially partition ability, and meanwhile some hms api
like `get_partitions_ps_with_auth` can allow `null `& `empty `, i think we can
check in `HiveMetaStoreClient` side, and it is good for me to unify the
behavior of common table and temp table.
##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreUtils.java:
##########
@@ -460,6 +460,26 @@ public static List<String> getPvals(List<FieldSchema>
partCols,
}
return pvals;
}
+
+ /**
+ * If all the values of partVals are empty strings, it means we are returning
+ * all the partitions and hence we can use get_partitions API.
+ * @param partVals The partitions values used to filter out the partitions.
+ * @return true if partVals is empty or if all the values in partVals is
empty strings.
+ * other wise false.
+ */
+ public static boolean arePartValsEmpty(List<String> partVals) {
+ if (partVals == null || partVals.isEmpty()) {
+ return true;
+ }
+ for (String val : partVals) {
+ if (val != null && !val.isEmpty()) {
Review Comment:
Why we did this check?
In which case, the val will be equal the `null `or `empty`?
##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreUtils.java:
##########
@@ -460,6 +460,26 @@ public static List<String> getPvals(List<FieldSchema>
partCols,
}
return pvals;
}
+
+ /**
+ * If all the values of partVals are empty strings, it means we are returning
+ * all the partitions and hence we can use get_partitions API.
+ * @param partVals The partitions values used to filter out the partitions.
+ * @return true if partVals is empty or if all the values in partVals is
empty strings.
+ * other wise false.
+ */
+ public static boolean arePartValsEmpty(List<String> partVals) {
+ if (partVals == null || partVals.isEmpty()) {
+ return true;
+ }
+ for (String val : partVals) {
+ if (val != null && !val.isEmpty()) {
+ return false;
+ }
+ }
+ return true;
+ }
Review Comment:
```suggestion
public static boolean arePartValsEmpty(List<String> partVals) {
return partVals == null || partVals.stream().allMatch(val -> val == null
|| val.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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
