HIVE-28655: Implement HMS Related Drop Stats Changes #5578

DanielZhu58 · 2024-12-16T01:38:29Z

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?

dengzhhu653 · 2024-12-16T03:32:13Z

standalone-metastore/metastore-common/src/main/thrift/hive_metastore.thrift

  bool delete_table_column_statistics(1:string db_name, 2:string tbl_name, 3:string col_name, 4:string engine) throws
              (1:NoSuchObjectException o1, 2:MetaException o2, 3:InvalidObjectException o3,
               4:InvalidInputException o4)
+  bool delete_table_column_statistics_req(1: DeleteTableColumnStatisticsRequest req) throws


can we merge the two methods? i.e, merge them to bool delete_column_statistics_req(DeleteColumnStatisticsRequest req)

dengzhhu653 · 2024-12-16T03:39:11Z

...astore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/IMetaStoreClient.java

+   *            catalog name, database name, table name, partition name, column names, and engine name
+   * @throws TException thrift transport error
+   */
+  public boolean deletePartitionMultiColumnStatistics(DeletePartitionColumnStatisticsRequest req) throws TException;


I would like to merge the deletePartitionMultiColumnStatistics and deleteTableMultiColumnStatistics, and keep only one method, i.e, a deleteMultiColumnStatistics(DeleteColumnStatisticsRequest req) for both dropping table statistics and partition statistics.

Sometimes the user just want to drop the stats in a specific partition.
In some cases, a certain partition stats becomes really huge so we want to drop stats for it only.
The 2 methods are designed for different use cases.

dengzhhu653 · 2024-12-16T03:44:17Z

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

+   * @throws InvalidObjectException error dropping the stats
+   * @throws InvalidInputException bad input, such as null table or database name.
+   */
+  boolean deletePartitionColumnStatisticsInBatch(String catName, String dbName, String tableName,


what's the difference between deletePartitionMultiColumnStatistics and deletePartitionColumnStatisticsInBatch, can them merge?

Yes, I agree. I think we can merge these 2 methods.

dengzhhu653 · 2024-12-16T03:44:43Z

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

@@ -1395,9 +1395,50 @@ List<ColumnStatistics> getPartitionColumnStatistics(
   * @throws InvalidInputException bad input, such as null table or database name.
   */
  boolean deletePartitionColumnStatistics(String catName, String dbName, String tableName,


you can make this method as default

Acknowledged.

dengzhhu653 · 2024-12-16T03:45:12Z

...store/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/cache/CachedStore.java

-import java.util.Optional;
-import java.util.Set;
-import java.util.Stack;
+import java.util.*;


nit: restore the import

Acknowledged.

dengzhhu653 · 2024-12-16T03:50:47Z

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

+
+    getMS().openTransaction();
+    try {
+      List<String> partVals = getPartValsFromName(getMS(), parsedDbName[CAT_NAME], parsedDbName[DB_NAME], tableName, partName);


this partName in thrift defines as optional, should we take care of the nullable partName. To make the interface more generic, can we define the partName as List<String>, so we can delete multiple partitions' statistics?

Currently we don't need to delete the stats for multiple partitions, because currently there is no syntax for the user to drop stats for multiple partitions in HQL.
And also in the use cases, it's not very common that stats of multiple adjacent partitions become huge. The user can drop stats of multiple partitions one by one manually.
I think this partName here is not optional.
In hive_metastore.thrift,
struct DeletePartitionColumnStatisticsRequest { 1: required string cat_name, 2: required string db_name, 3: required string tbl_name, 4: required string part_name, 5: optional list<string> col_names, 6: required string engine }

dengzhhu653 · 2024-12-16T03:53:26Z

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

+      Table table = getMS().getTable(parsedDbName[CAT_NAME], parsedDbName[DB_NAME], tableName);
+      // This API looks unused; if it were used we'd need to update stats state and write ID.
+      // We cannot just randomly nuke some txn stats.
+      if (TxnUtils.isTransactionalTable(table)) {


why can't we drop the transactional statistics? can we make the partition stats as incorrect in partition params by StatsSetupConst.COLUMN_STATS_ACCURATE

This comments are not written by me. We can mark this issue as to do, and discuss about it later.

dengzhhu653 · 2024-12-16T03:56:00Z

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

+                    new DeletePartitionColumnStatEvent(parsedDbName[CAT_NAME], parsedDbName[DB_NAME], tableName,
+                            partName, partVals, colName, engine, this));
+          }
+          if (!listeners.isEmpty()) {


should this call be outside of the transaction? why do we notify the listener just on success and colNames is not null, will some events be missing?

dengzhhu653 · 2024-12-16T03:59:45Z

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

+        throw new NoSuchObjectException("Partition " + partName
+                + " for which stats deletion is requested doesn't exist");
+      }
+      query = pm.newQuery(MPartitionColumnStatistics.class);


where do we use this query?

dengzhhu653 · 2024-12-16T04:00:32Z

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

+      query = pm.newQuery(MPartitionColumnStatistics.class);
+      if (colNames != null) {
+        for (String colName : colNames){
+          deletePartitionColumnStatistics(catName, dbName, tableName, partName, partVals, colName, engine);


can we drop all the columns' statistics in a round?

Aggarwal-Raghav · 2024-12-16T06:42:35Z

@DanielZhu58, the thrift code needs to be generated as there is change in hive_metastore.thrift. Please use -Pthriftif profile.

sonarqubecloud · 2024-12-18T12:50:45Z

Quality Gate passed

Issues
27 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

Aggarwal-Raghav · 2024-12-23T06:59:41Z

@DanielZhu58, one question, to call the drop stats command it will be something like: ALTER TABLE .. drop col/partition stats... So antlr file changes (HiveParser.g) are required right?

DanielZhu58 · 2025-01-03T02:08:28Z

@DanielZhu58, one question, to call the drop stats command it will be something like: ALTER TABLE .. drop col/partition stats... So antlr file changes (HiveParser.g) are required right?

Hi @Aggarwal-Raghav , yes, the method changes in this patch are mainly focused on the HMS API changes. It's only the part of the big change.
There is another patch included the HiveParser.g changes coming soon, authored by @ramesh0201
#5176

Aggarwal-Raghav · 2025-01-03T06:46:17Z

@DanielZhu58, one question, to call the drop stats command it will be something like: ALTER TABLE .. drop col/partition stats... So antlr file changes (HiveParser.g) are required right?

Hi @Aggarwal-Raghav , yes, the method changes in this patch are mainly focused on the HMS API changes. It's only the part of the big change. There is another patch included the HiveParser.g changes coming soon, authored by @ramesh0201 #5176

ok

dengzhhu653 · 2025-01-15T10:00:10Z

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

+      MTable mTable = getMTable(catName, dbName, tableName);
+      MPartitionColumnStatistics mStatsObj;
+      List<MPartitionColumnStatistics> mStatsObjColl;
+      if (mTable == null) {


Is this check mTable == null redundant? as we validate the mPartition afterwards

I think we can keep this if condition.
Because if mTable is null, the exception will be thrown from here, so we can know the "Table" does not exist.
If the exception is thrown later with the mPartition, we can know the "Partition" does not exist.

dengzhhu653 · 2025-01-15T10:00:51Z

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

+      query = pm.newQuery(MPartitionColumnStatistics.class);
+      String filter;
+      String parameters;
+      if (colNames != null) {


nit: colNames != null && !colNames.isEmpty()

Acknowledged.

dengzhhu653 · 2025-01-15T10:02:36Z

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

+      query.setFilter(filter);
+      query.declareParameters(parameters);
+      if (colNames != null) {
+        query.setUnique(true);


as we might have multiple columns, there is no guarantee that the query will return only one row, can we just remove it?

Acknowledged. Yes, this should be removed.

dengzhhu653 · 2025-01-15T10:04:49Z

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

+        query.setUnique(true);
+        for (String colName : colNames){
+          // trim the extra spaces, and change to lowercase
+          normalizeIdentifier(colName);


need to put the normalized column names into new list which will be passed to executeWithArray

Acknowledged. Created a new list normalizedColNames, to pass to executeWithArray.

dengzhhu653 · 2025-01-15T10:05:33Z

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

+          normalizeIdentifier(colName);
+        }
+        if (engine != null) {
+          mStatsObj =


this query result should be a collection

Acknowledged.

dengzhhu653 · 2025-01-15T10:06:54Z

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

+                  + TableName.getQualified(catName, dbName, tableName) +
+                  " partition=" + partName + " col=" + String.join(", ", colNames));
+        }
+      } else {


nit: can we merge the if else?

One condition is (colNames != null) and the other one condition is (colNames == null). In this 2 conditions, the JDO queries are different. So it's clearer to not merge them for now.

dengzhhu653 · 2025-01-15T10:09:02Z

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

+                  + TableName.getQualified(catName, dbName, tableName) + " partition" + partName);
+        }
+      }
+      ret = commitTransaction();


after deleting the column stats, should we mark the COLUMN_STATS_ACCURATE to false in partition's parameters?

That's a good point. I don't have an answer for it now.
We had the feature of dropping column stats for single column before, and the COLUMN_STATS_ACCURATE was not mark to false.
So let's discuss about that later.

dengzhhu653 · 2025-01-15T10:09:42Z

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

+      query = pm.newQuery(MTableColumnStatistics.class);
+      String filter;
+      String parameters;
+      if (colNames != null) {


nit: colNames != null and !colNames.isEmpty()

Acknowledged.

dengzhhu653 · 2025-01-15T10:10:11Z

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

+      query.setFilter(filter);
+      query.declareParameters(parameters);
+      if (colNames != null) {
+        query.setUnique(true);


remove the unique check

Acknowledged.

dengzhhu653 · 2025-01-15T10:10:40Z

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

+        query.setUnique(true);
+        for (String colName : colNames){
+          // trim the extra spaces, and change to lowercase
+          normalizeIdentifier(colName);


put the normalize into a new list

Acknowledged.

dengzhhu653 · 2025-01-15T10:11:08Z

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

+          throw new NoSuchObjectException("Column stats doesn't exist for db=" + dbName + " table="
+                  + tableName + " col=" + String.join(", ", colNames));
+        }
+      } else {


merge the if else

Reasons same as mentioned above.

dengzhhu653 · 2025-01-16T08:43:53Z

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

+        }
+        pm.retrieveAll(mStatsObjColl);
+        if (mStatsObjColl != null) {
+          pm.deletePersistentAll(mStatsObjColl);
        } else {
          throw new NoSuchObjectException("Column stats doesn't exist for table="


nit: this exception should never throw?

Let's discuss this today. I am not sure if I understand this.

dengzhhu653 · 2025-01-16T08:44:36Z

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

-                  normalizeIdentifier(tableName),
-                  normalizeIdentifier(catName),
-                  engine);
+                  (List<MPartitionColumnStatistics>) query.executeWithArray(partName.trim(),


nit: code format

Acknowledged.

dengzhhu653 · 2025-01-16T08:45:17Z

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

+   * @throws InvalidInputException bad input, such as null table or database name.
+   */
+  boolean deletePartitionMultiColumnStatistics(String catName, String dbName, String tableName,
+    String partName, List<String> partVals, List<String> colNames, String engine)


the partVals never be used?

It's used around ObjectStore.java Line 10266

dengzhhu653 · 2025-01-16T08:48:41Z

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

+    if (colName != null) {
+      colNames.add(colName);
+    }
+    return deletePartitionMultiColumnStatistics(catName, dbName, tableName, partName, partVals, colNames, engine);


The direct sql path is missing, we'd better pushing the colNames into the direct sql, example:
directSql.deletePartitionColumnStats(catName, dbName, tableName, partName, colNames, engine)

Let's discuss this today. I am not sure if I understand this.

dengzhhu653 · 2025-01-16T08:51:20Z

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

+  default boolean deletePartitionColumnStatistics(String catName, String dbName, String tableName,
+    String partName, List<String> partVals, String colName, String engine)
+    throws NoSuchObjectException, MetaException, InvalidObjectException, InvalidInputException{
+    return false;


return deletePartitionMultiColumnStatistics(catName, dbName, tableName,
partName, partVals, Arrays.asList(colName), engine);

Acknowledged.

dengzhhu653 · 2025-01-16T08:55:14Z

...store/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetaStore.java

+              dbName, tblName, Lists.newArrayList(colName[1]), ENGINE);
+      assertTrue("stats are not empty: " + stats, stats.isEmpty());
+
+      client.updateTableColumnStatistics(colStats);


we have tested the single column case at https://github.com/apache/hive/pull/5578/files#diff-120e8efba348e9a8c233de4135cdcc4aaeb6bc0fe2540d9d952554af2c05fdecR1867

dengzhhu653 · 2025-01-16T08:56:24Z

...store/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetaStore.java

+      //case 3: column names contains multiple column names, then delete the column stats in the column name list
+      status = client.deleteTableMultiColumnStatistics(dbName, tblName, Arrays.asList(colName), ENGINE);
+      assertTrue(status);
+      stats = client.getTableColumnStatistics(


get the stats in a round, client.getTableColumnStatistics(dbName, tblName, Lists.newArrayList(colName), ENGINE);

dengzhhu653 · 2025-01-16T08:56:56Z

...store/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetaStore.java

-         Lists.newArrayList(partName), Lists.newArrayList(colName[1]), ENGINE);
-     assertTrue("stats are not empty: " + stats2, stats2.isEmpty());
+      // compare stats obj to ensure what we get is what we wrote
+      assertNotNull(colStats2);


restore the code format

dengzhhu653 · 2025-01-16T08:59:45Z

...store/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetaStore.java

+      // case 1: column names are null, all column stats should be deleted
+      status = client.deletePartitionMultiColumnStatistics(dbName, tblName, partName, null, ENGINE);
+      assertTrue(status);
+      stats2 = client.getPartitionColumnStatistics(dbName, tblName,


nit: get the all column stats in a round

asf-ci-hive added tests pending tests failed and removed tests pending labels Dec 16, 2024

dengzhhu653 reviewed Dec 16, 2024

View reviewed changes

DanielZhu58 force-pushed the HIVE-28655 branch from e6bfa1f to 3cc3276 Compare December 18, 2024 11:25

asf-ci-hive added tests pending and removed tests failed labels Dec 18, 2024

asf-ci-hive added tests unstable and removed tests pending labels Dec 18, 2024

DanielZhu58 force-pushed the HIVE-28655 branch from 3cc3276 to dcd1438 Compare January 14, 2025 04:16

asf-ci-hive added tests pending tests failed and removed tests unstable tests pending labels Jan 14, 2025

DanielZhu58 force-pushed the HIVE-28655 branch from dcd1438 to 9ff54ca Compare January 14, 2025 09:12

asf-ci-hive removed the tests failed label Jan 14, 2025

asf-ci-hive added tests pending tests failed and removed tests failed tests pending labels Jan 15, 2025

dengzhhu653 reviewed Jan 15, 2025

View reviewed changes

HIVE-28655: Implement HMS Related Drop Stats Changes

68a5561

DanielZhu58 force-pushed the HIVE-28655 branch from bb24404 to 68a5561 Compare January 16, 2025 08:01

asf-ci-hive added tests pending tests failed and removed tests failed tests pending labels Jan 16, 2025

dengzhhu653 reviewed Jan 16, 2025

View reviewed changes

HIVE-28655: Implement HMS Related Drop Stats Changes #5578

Are you sure you want to change the base?

HIVE-28655: Implement HMS Related Drop Stats Changes #5578

Conversation

DanielZhu58 commented Dec 16, 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

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

Aggarwal-Raghav commented Dec 16, 2024

sonarqubecloud bot commented Dec 18, 2024

Quality Gate passed

Aggarwal-Raghav commented Dec 23, 2024

DanielZhu58 commented Jan 3, 2025

Aggarwal-Raghav commented Jan 3, 2025

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 Jan 15, 2025 • edited Loading

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

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 Jan 16, 2025 • edited Loading

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 Jan 15, 2025 •

edited

Loading

dengzhhu653 Jan 16, 2025 •

edited

Loading