HIVE-29694 by InvisibleProgrammer · Pull Request #6577 · apache/hive

InvisibleProgrammer · 2026-07-03T13:49:49Z

Details in the corresponding Jira: https://issues.apache.org/jira/browse/HIVE-29694

Notes about the implementation:
MetaStoreDirectSql.getColumnDescriptor runs multiple queries to get a reusable column descriptor.
I leave the possibility to reduce the amount of queries run as a future improvement as theoretically it would be possible to find a single query that covers all the candidates but practically Hive support so many HMS databases and their feature set is significant.

What changes were proposed in this pull request?

The problem came from replication when addPartition added a lot of extra records in case the table had schema evolution.
In this change, we try to re-use existing schema info based on the following algorithm:

Take the latest 10 partition
Filter out partitions when the column count matches with the column count of the partition that we currently want to add.
Check the names and types of the candidates in order. If there is a math, we can reuse that info. If not, we can still add a new record.

Why are the changes needed?

We got a customer with huge tables (with hundreds of thousands of partitions) with schema evolution between partitions. Their HMS became bloated.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Added new test class, TestHMSColumnDescriptorReuse.
Ran it with all the supported databases.

dengzhhu653 · 2026-07-06T02:56:43Z

    ACID_TXN_CLEANER_INTERVAL("metastore.acid.txn.cleaner.interval",
        "hive.metastore.acid.txn.cleaner.interval", 10, TimeUnit.SECONDS,
        "Time interval describing how often aborted and committed txns are cleaned."),
+    ADD_PARTITION_REUSE_EXISTING_COLUMN_DESCRIPTORS("metastore.add.partition.reuse.existing.column.descriptors",


since this is an optimization for large tables, for small tables, it shouldn't introduce too much overhead, should we set the default to true?

This code got a lot of modifications since your last review. What do you think, should we still turn it true, by default?

I think we should still make it true by default

A few thoughts on simplifying the config name (it seems verbose) and the functionality, default value:

the word 'existing' is already implied by 'reuse' since one can only reuse existing descriptors, so you could remove 'existing'.

would this functionality only apply for explicit ADD PARTITION commands or would it also apply for MSCK REPAIR which also adds partitions under the covers that are missing in the metastore but are present in the filesystem. Once MSCK REPAIR adds partitions, then subsequent schema evolutions of those partitions should ideally also be reusing the column descriptors. But if this is not currently supported or tested, it is ok to file a separate ticket for followup.

from an end user perspective, ADD COLUMNS and CHANGE COLUMN are also actions that trigger this duplicate column descriptors, so it may not be accurate to say 'add.partition' . Considering all these, we could simplify it to metastore.partition.reuse.column.descriptors. WDYT ?

About the default value, since this is changing the generated SQL, it would be better to give it some more time for testing - not just functionally but also performance with order of 100K partitions. Metastore sql changes have significant impact and is in some sense a behavior change, so would be good to wait for further testing.

metastore.partition.reuse.column.descriptors sounds fine. Thank you.

dengzhhu653 · 2026-07-07T04:03:27Z

I have moved convertToMPart to the TableStoreImpl, so we don't need create an extra interface or pass the RawStore

InvisibleProgrammer · 2026-07-07T19:02:24Z

+
+      List<Long> latestColumnDescriptorIds = new  ArrayList<>();
+      for (Object cdId : sqlResult) {
+        latestColumnDescriptorIds.add(extractLongValue(cdId));


@dengzhhu653 : I introduced `extractLongValue as the type of CD_ID is number in Oracle and executeWithArray returns with BigDecimal instead of Long.

Do we have an existing reusable code to handle the situation?

you can use MetastoreDirectSqlUtils.extractSqlLong instead

dengzhhu653 · 2026-07-07T23:22:34Z

+
+        @Override
+        protected MColumnDescriptor getSqlResult() throws MetaException {
+          if (MetastoreConf.getBoolVar(conf, ADD_PARTITION_REUSE_EXISTING_COLUMN_DESCRIPTORS)) {


can you check this boolean property in the else if branch with if (cols.equals(tableSchema)) {?
else if (MetastoreConf.getBoolVar(conf, ADD_PARTITION_REUSE_EXISTING_COLUMN_DESCRIPTORS) {

dengzhhu653 · 2026-07-07T23:24:26Z

+    // that's the easy and relatively fast-check since does not require
+    // round-tripe to the database
+    List<FieldSchema> tableSchema = mt.getSd() != null && mt.getSd().getCD() != null && mt.getSd()
+        .getCD()


nit: code format, change the mt.getSd() .getCD() .getCols() != null to one line

It would violate the 120 long rule of the static code analyzer. However, I reformatted that statement to get better readability.

dengzhhu653 · 2026-07-07T23:29:00Z

+
+    } finally {
+      if (query != null) {
+        query.closeAll();


nit: don't need to close the query any more, the TransactionHandler could take care of resource releasing

dengzhhu653 · 2026-07-07T23:34:24Z

+              break;
+            }
+          }
+          return candidate;


this could return a mistaken candidate even if !cols.get(i).equals(col)

dengzhhu653 · 2026-07-07T23:35:48Z

+    // Please note! In case you modify any of those methods, run TestHMSColumnDescriptorReuse with all the supported databases
+    List<Long> cdCandidates = findTheLatestColumnDescriptors(tblId);
+
+    cdCandidates = filterCandidatesByColumnCount(cdCandidates, cols.size());


return early if cdCandidates is null

dengzhhu653 · 2026-07-07T23:36:22Z

+  }
+
+  private List<Long> filterCandidatesByColumnCount(List<Long> cdCandidates, int size) throws MetaException {
+    Query<?> query = null;


the cdCandidates might be null

dengzhhu653 · 2026-07-07T23:37:01Z

 import org.apache.hadoop.hive.metastore.utils.MetaStoreUtils;
 import org.apache.hive.common.util.BloomFilter;
 import org.datanucleus.store.rdbms.query.ForwardQueryResult;
+import org.jetbrains.annotations.Nullable;


nit: unnecessary import

dengzhhu653 · 2026-07-07T23:45:29Z

+        JOIN "SDS" s ON s."SD_ID" = p."SD_ID"
+        WHERE p."TBL_ID" = ?
+        GROUP BY s."CD_ID"
+        ORDER BY MAX(p."PART_ID") DESC


may be we can add a limit here for saving time, e.g, limit 20, most recent schemas have the biggest match usually

I cannot add a limit clause that is compatible with all the supported databases. However, I can add a limit to the executeWithArray call so that it will only read the first 20 records.

dengzhhu653 · 2026-07-07T23:55:11Z

+    return convertToMStorageDescriptor(sd, mcd);
+  }
+
+  @VisibleForTesting


nit: since we don't use this annotation, can we remove it?

amansinha100

Thanks for working in this PR. I mainly had comments about the config and related functionality.

amansinha100 · 2026-07-08T01:41:31Z

    ACID_TXN_CLEANER_INTERVAL("metastore.acid.txn.cleaner.interval",
        "hive.metastore.acid.txn.cleaner.interval", 10, TimeUnit.SECONDS,
        "Time interval describing how often aborted and committed txns are cleaned."),
+    ADD_PARTITION_REUSE_EXISTING_COLUMN_DESCRIPTORS("metastore.add.partition.reuse.existing.column.descriptors",


A few thoughts on simplifying the config name (it seems verbose) and the functionality, default value:

the word 'existing' is already implied by 'reuse' since one can only reuse existing descriptors, so you could remove 'existing'.

would this functionality only apply for explicit ADD PARTITION commands or would it also apply for MSCK REPAIR which also adds partitions under the covers that are missing in the metastore but are present in the filesystem. Once MSCK REPAIR adds partitions, then subsequent schema evolutions of those partitions should ideally also be reusing the column descriptors. But if this is not currently supported or tested, it is ok to file a separate ticket for followup.

from an end user perspective, ADD COLUMNS and CHANGE COLUMN are also actions that trigger this duplicate column descriptors, so it may not be accurate to say 'add.partition' . Considering all these, we could simplify it to metastore.partition.reuse.column.descriptors. WDYT ?

About the default value, since this is changing the generated SQL, it would be better to give it some more time for testing - not just functionally but also performance with order of 100K partitions. Metastore sql changes have significant impact and is in some sense a behavior change, so would be good to wait for further testing.

…ases

Copilot

Pull request overview

This PR adds an opt-in mechanism to reuse existing HMS column descriptors when adding partitions (to reduce metadata bloat during partition-level schema evolution, especially in replication scenarios), plus targeted direct-SQL support and a new unit test to validate the behavior.

Changes:

Add a new metastore config (metastore.partition.reuse.column.descriptors) and wire partition conversion to reuse matching column descriptors when enabled.
Implement DirectSQL lookup logic to find and reuse a suitable existing MColumnDescriptor based on recent partitions.
Add TestHMSColumnDescriptorReuse to exercise reuse behavior across schema evolution cases.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestHMSColumnDescriptorReuse.java	Adds unit tests validating descriptor reuse across schema evolution and replication-like partition adds.
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/metastore/impl/TableStoreImpl.java	Routes partition SD conversion through a reusable column-descriptor lookup (when enabled).
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/directsql/MetastoreDirectSqlUtils.java	Adjusts SQL string extraction to handle Oracle/DataNucleus empty-string workaround values.
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/directsql/MetaStoreDirectSql.java	Adds DirectSQL implementation for finding a reusable column descriptor from recent partitions.
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java	Introduces the new configuration flag for enabling descriptor reuse (and reorders one existing conf var).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+  }
+
+  private List<Long> findTheLatestColumnDescriptors(long tblId) throws MetaException {
+    final int limit = 20;


+            Object[] row = rows.get(i);
+            FieldSchema col = new FieldSchema(
+                extractSqlString(row[0]),
+                extractSqlClob(row[1]),
+                extractSqlString(row[2]));
+            if (!cols.get(i).equals(col)) {
+              break;
+            }


dengzhhu653 · 2026-07-09T03:00:48Z

Pushed a minor refactor to address the code-format from Copilot and Sonar and restart the CI test.

sonarqubecloud · 2026-07-09T04:04:23Z

Quality Gate passed

Issues
7 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud

DanielZhu58 · 2026-07-09T08:19:15Z

I’m generally supportive of this PR.
The approach of fixing this on the add-partition/import path is better than only optimizing the read path later. By trying to reuse an existing column descriptor before creating a new CD_ID, this can prevent metadata bloat.
I also support that the change is guarded by a new config.

A couple of things I think we might need to pay attention later:
We only checks a limited set of recent CD_IDs, so we should document that clearly.
We should also test it across all supported metastore DBs, (does it work with oracle?) and consider a batch-level cache later to reduce repeated DirectSQL lookups during large imports.
We can do the a possible follow-up dedup tool later.

Overall, I think this is a good and working fix.

asf-ci-hive added the tests pending label Jul 3, 2026

InvisibleProgrammer mentioned this pull request Jul 3, 2026

HIVE-29694: Reuse DB column descriptors when adding partitions in HMS #6574

Draft

asf-ci-hive added tests unstable and removed tests pending labels Jul 3, 2026

dengzhhu653 reviewed Jul 6, 2026

View reviewed changes

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

dengzhhu653 reviewed Jul 6, 2026

View reviewed changes

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

InvisibleProgrammer force-pushed the HIVE-29694 branch from c307567 to a7bb0eb Compare July 7, 2026 14:58

asf-ci-hive added tests pending and removed tests unstable labels Jul 7, 2026

InvisibleProgrammer force-pushed the HIVE-29694 branch from a7bb0eb to ce44ed1 Compare July 7, 2026 15:25

asf-ci-hive added tests failed and removed tests pending labels Jul 7, 2026

InvisibleProgrammer force-pushed the HIVE-29694 branch from ce44ed1 to 988483d Compare July 7, 2026 16:01

InvisibleProgrammer commented Jul 7, 2026

View reviewed changes

InvisibleProgrammer marked this pull request as ready for review July 7, 2026 19:03

asf-ci-hive added tests pending tests unstable and removed tests failed tests pending labels Jul 7, 2026

dengzhhu653 reviewed Jul 7, 2026

View reviewed changes

amansinha100 reviewed Jul 8, 2026

View reviewed changes

asf-ci-hive added tests pending and removed tests unstable labels Jul 8, 2026

zabetak and others added 6 commits July 8, 2026 17:15

HIVE-29694: Reuse DB column descriptors when adding partitions in HMS

9481e0c

HIVE-29694: Add feature flag

f9935d6

WiP: modify getColumnDescriptor

e8fa1dd

Fixes to make it compatible for all supported databases

0bb33ef

Addressed review comments, code style issues, bugfix and extra test c…

85c05e1

…ases

Fix DataNucleus' workaround

3df1ec4

InvisibleProgrammer force-pushed the HIVE-29694 branch from 2d9d504 to 3df1ec4 Compare July 8, 2026 15:15

asf-ci-hive added tests passed tests pending tests unstable and removed tests pending tests passed labels Jul 8, 2026

dengzhhu653 requested a review from Copilot July 9, 2026 02:25

Copilot started reviewing on behalf of dengzhhu653 July 9, 2026 02:25 View session

Copilot AI reviewed Jul 9, 2026

View reviewed changes

re-format

d5a5157

asf-ci-hive added tests pending and removed tests unstable labels Jul 9, 2026

dengzhhu653 approved these changes Jul 9, 2026

View reviewed changes

asf-ci-hive added tests passed and removed tests pending labels Jul 9, 2026

Uh oh!

Conversation

InvisibleProgrammer commented Jul 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dengzhhu653 commented Jul 7, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amansinha100 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dengzhhu653 commented Jul 9, 2026

Uh oh!

sonarqubecloud Bot commented Jul 9, 2026

Quality Gate passed

Uh oh!

DanielZhu58 commented Jul 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

InvisibleProgrammer commented Jul 3, 2026 •

edited

Loading