HIVE-28205: Implement direct sql for get_partitions_ps_with_auth api #5206

wecharyu · 2024-04-21T12:41:38Z

What changes were proposed in this pull request?

Implement direct sql for getNumPartitionsByPs and listPartitionsPsWithAuth.

Why are the changes needed?

To improve performance.

Does this PR introduce any user-facing change?

No.

Is the change a dependency upgrade?

No.

How was this patch tested?

Passing existing tests and add benchmark test benchmarkGetPartitionsByPs:

java -jar hmsbench.jar -H localhost --savedata /tmp/benchdata --sanitize -N 100 -N 1000 -o bench_results.csv -C -d testbench_http --params=100 -M 'getPartitionsByPs.*'

Before this patch

Operation       Mean    Med     Min     Max     Err%
getPartitionsByPs       14.0807 13.9823 12.8813 19.2509 5.63364
getPartitionsByPs.100   276.291 275.259 267.642 301.320 2.33622
getPartitionsByPs.1000  2631.50 2631.01 2551.76 2991.55 1.86420

After this patch

Operation       Mean    Med     Min     Max     Err%
getPartitionsByPs       11.6247 11.2780 10.7851 23.8686 12.7184
getPartitionsByPs.100   18.3550 18.0011 17.1338 36.5164 10.8893
getPartitionsByPs.1000  61.5760 60.6764 59.3697 78.2133 5.14500

wecharyu · 2024-04-24T16:37:56Z

...tastore-server/src/test/java/org/apache/hadoop/hive/metastore/client/TestListPartitions.java

@@ -409,10 +409,13 @@ public void testListPartitionsByValues() throws Exception {
    assertEquals(0, partitions.size());
  }

-  @Test(expected = MetaException.class)


Empty part_vals should not throw MetaException because we support it in ObjectStore and regard it as fetching all partitions:

hive/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java

Lines 3890 to 3893 in 2d855b2

private boolean canTryDirectSQL(List<String> partVals) {

if (partVals == null || partVals.isEmpty()) {

return true;

}

...tore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java

dengzhhu653 · 2024-04-28T03:09:59Z

...tore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java

+    return getNumPartitionsViaSql(queryText, params);
+  }
+
+  private int getNumPartitionsViaSql(String queryText, Object[] params) {


can we rename this method to getCountOfQuery and move it to MetastoreDirectSqlUtils?

...ore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java

dengzhhu653 · 2024-04-28T03:15:18Z

ql/src/java/org/apache/hadoop/hive/ql/metadata/PartitionTree.java

@@ -121,8 +122,8 @@ List<Partition> addPartitions(List<Partition> partitions, boolean ifNotExists)
   * {@link MetaStoreUtils#getPvals(List, Map)}
   */
  List<Partition> getPartitionsByPartitionVals(List<String> partialPartVals) throws MetaException {
-    if (partialPartVals == null || partialPartVals.isEmpty()) {
-      throw new MetaException("Partition partial vals cannot be null or empty");
+    if (MetaStoreUtils.arePartValsEmpty(partialPartVals)) {


same as above

Add null check in SessionHiveMetaStoreClient, here we can follow the server side behavior to support empty values.

.../metastore-tools/tools-common/src/main/java/org/apache/hadoop/hive/metastore/tools/Util.java

dengzhhu653 · 2024-04-28T03:26:58Z

.../metastore-tools/tools-common/src/main/java/org/apache/hadoop/hive/metastore/tools/Util.java

@@ -584,15 +584,30 @@ static List<Partition> createManyPartitions(@NotNull Table table,
                                              @Nullable Map<String, String> parameters,
                                              @NotNull List<String> arguments,
                                              int npartitions) {
-    return IntStream.range(0, npartitions)


why split this method?

...metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetastoreDirectSqlUtils.java

...e-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java

dengzhhu653 · 2024-05-15T08:25:12Z

ql/src/java/org/apache/hadoop/hive/ql/metadata/PartitionTree.java

@@ -22,6 +22,7 @@
 import org.apache.hadoop.hive.metastore.api.MetaException;
 import org.apache.hadoop.hive.metastore.api.NoSuchObjectException;
 import org.apache.hadoop.hive.metastore.api.Partition;
+import org.apache.hadoop.hive.metastore.utils.MetaStoreUtils;


nit: can we revert the import as well?

...metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetastoreDirectSqlUtils.java

sonarcloud · 2024-05-16T05:06:42Z

Quality Gate passed

Issues
14 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

dengzhhu653 · 2024-05-17T09:53:59Z

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


what if the table is a temporary table while the partVals is null?

It would throw MetaException like HiveMetaStoreClient.

Why not revert back the check on PartitionTree.java, it's a bit confusing a temporary table just performs the null check in HiveMetaStoreClient

Because SessionHiveMetaStoreClient extends HiveMetaStoreClient, it should also check null for part_vals like HiveMetaStoreClient, and PartitionTree should support empty or even null part_vals like metastore server in my opinion.

Otherwise it would cause the inconsistent test results in TestListPartitions for HiveMetaStoreClient and SessionHiveMetaStoreClient, for example testListPartitionsByValuesNoVals(). The master branch mark this test expect MetaException, but actually it should return all partitions for empty part_vals. It could pass the test in master because it throws MetaException when making part name for empty partVals in getNumPartitionsByPs, which should be a bug in implement this method.

Because SessionHiveMetaStoreClient extends HiveMetaStoreClient, it should also check null for part_vals like HiveMetaStoreClient

I think you cannot just push the null check of a temporary table in HiveMetaStoreClient, and the PartitionTree is for the temporary table to process the partition request, it works at the client side, why not put the check here?

the PartitionTree is for the temporary table to process the partition request, it works at the client side, why not put the check here?

Because get_partitions_ps_with_auth support null and empty part_vals, the null check is added in client side. So in PartitionTree we can also follow the server behavior and push null check in SessionHiveMetaStoreClient. This change seems to have no side effects, and in turn supports empty part_vals like HiveMetaStoreClient.

I don't like this idea: null check for temporary table in HiveMetaStoreClient, let check others' opinion. cc @deniskuzZ @zhangbutao

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.

but i see that two of them are not changed

One is that SessionHiveMetaStoreClient#istPartitionNamesRequest does not check null for part_vals, I think it's acceptable because HiveMetaStoreClient#listPartitionNamesRequest also not check it.

The other is SessionHiveMetaStoreClient#exchangePartitions where part_vals can not be null.

I can not find out the side effect on enable null or empty part_vals in these calls if we do regard null or empty part_vals is used to fetch all parts or part names.

zhangbutao · 2024-05-22T07:27:37Z

...re/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreUtils.java

+  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;
+  }


Suggested change

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;

}

public static boolean arePartValsEmpty(List<String> partVals) {

return partVals == null || partVals.stream().allMatch(val -> val == null || val.isEmpty());

}

Good suggestion, will update together with the final discussion result of part_val null check.

zhangbutao · 2024-05-22T07:47:04Z

...re/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreUtils.java

+      return true;
+    }
+    for (String val : partVals) {
+      if (val != null && !val.isEmpty()) {


Why we did this check?

In which case, the val will be equal the null or empty?

It's existing code, the val is determined in client side.

zhangbutao · 2024-05-22T09:09:17Z

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


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.

asf-ci-hive added tests pending tests unstable and removed tests pending labels Apr 21, 2024

wecharyu force-pushed the HIVE-28205 branch from 6ca806b to dcb12bc Compare April 24, 2024 16:31

asf-ci-hive added tests pending and removed tests unstable labels Apr 24, 2024

wecharyu commented Apr 24, 2024

View reviewed changes

asf-ci-hive added tests unstable and removed tests pending labels Apr 24, 2024

wecharyu force-pushed the HIVE-28205 branch from dcb12bc to ec1b817 Compare April 25, 2024 07:46

asf-ci-hive added tests pending tests passed and removed tests unstable tests pending labels Apr 25, 2024

dengzhhu653 reviewed Apr 28, 2024

View reviewed changes

...tore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java Show resolved Hide resolved

dengzhhu653 reviewed Apr 28, 2024

View reviewed changes

...ore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java Outdated Show resolved Hide resolved

dengzhhu653 reviewed Apr 28, 2024

View reviewed changes

.../metastore-tools/tools-common/src/main/java/org/apache/hadoop/hive/metastore/tools/Util.java Outdated Show resolved Hide resolved

dengzhhu653 reviewed Apr 28, 2024

View reviewed changes

asf-ci-hive added tests pending tests passed and removed tests passed tests pending labels May 8, 2024

dengzhhu653 reviewed May 13, 2024

View reviewed changes

...metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetastoreDirectSqlUtils.java Outdated Show resolved Hide resolved

dengzhhu653 reviewed May 13, 2024

View reviewed changes

...e-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java Outdated Show resolved Hide resolved

asf-ci-hive added tests pending tests unstable and removed tests passed tests pending labels May 14, 2024

dengzhhu653 reviewed May 15, 2024

View reviewed changes

...metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetastoreDirectSqlUtils.java Outdated Show resolved Hide resolved

asf-ci-hive added tests pending tests unstable and removed tests unstable tests pending labels May 15, 2024

wecharyu added 6 commits May 16, 2024 03:56

HIVE-28205: Implement direct sql for get_partitions_ps_with_auth api

14b57ba

add benchmark test

4a6802d

fix tests

13038b5

address comments

33de824

revert null part_vals check in client

32c9dda

fix tests

db9b9dc

wecharyu force-pushed the HIVE-28205 branch from e998ff6 to db9b9dc Compare May 16, 2024 03:58

asf-ci-hive added tests pending and removed tests unstable labels May 16, 2024

asf-ci-hive added tests passed and removed tests pending labels May 16, 2024

dengzhhu653 reviewed May 17, 2024

View reviewed changes

zhangbutao reviewed May 22, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HIVE-28205: Implement direct sql for get_partitions_ps_with_auth api #5206

HIVE-28205: Implement direct sql for get_partitions_ps_with_auth api #5206

wecharyu commented Apr 21, 2024

wecharyu Apr 24, 2024

dengzhhu653 Apr 28, 2024

wecharyu May 8, 2024

dengzhhu653 Apr 28, 2024

wecharyu May 15, 2024

dengzhhu653 Apr 28, 2024

wecharyu May 8, 2024

dengzhhu653 May 15, 2024

sonarcloud bot commented May 16, 2024

dengzhhu653 May 17, 2024 •

edited

wecharyu May 17, 2024

dengzhhu653 May 20, 2024 •

edited

wecharyu May 20, 2024

dengzhhu653 May 20, 2024

wecharyu May 21, 2024

dengzhhu653 May 21, 2024

zhangbutao May 22, 2024

wecharyu May 22, 2024

zhangbutao May 22, 2024

wecharyu May 22, 2024

zhangbutao May 22, 2024

wecharyu May 22, 2024

zhangbutao May 22, 2024

	private boolean canTryDirectSQL(List<String> partVals) {
	if (partVals == null \|\| partVals.isEmpty()) {
	return true;
	}

HIVE-28205: Implement direct sql for get_partitions_ps_with_auth api #5206

Are you sure you want to change the base?

HIVE-28205: Implement direct sql for get_partitions_ps_with_auth api #5206

Conversation

wecharyu commented Apr 21, 2024

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

Is the change a dependency upgrade?

How was this patch tested?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented May 16, 2024

Quality Gate passed

dengzhhu653 May 17, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dengzhhu653 May 20, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dengzhhu653 May 17, 2024 •

edited

dengzhhu653 May 20, 2024 •

edited