[WIP]#6450
Conversation
There was a problem hiding this comment.
Pull request overview
This PR appears to refactor the standalone metastore RawStore into pluggable sub-stores (e.g., TableStore, PrivilegeStore, NotificationStore) wired via a new @MetaDescriptor + unwrap() mechanism, with transaction/query lifecycle managed by proxy/handlers. It also updates tests/utilities to use the new store interfaces and improves a few test/metadata details (e.g., catalog name in stats).
Changes:
- Introduces
MetaDescriptor-based store unwrapping plus transactional proxying (TransactionHandler,RawStoreAware,PersistenceManagerProxy). - Adds new store interfaces + implementations for table/privilege/notification operations and migrates
RawStoreAPIs to default delegations. - Updates several tests and utilities to use
TableStoreand a newDirectSqlConfiguratorhelper.
Reviewed changes
Copilot reviewed 20 out of 23 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/utils/DirectSqlConfigurator.java | New test helper to toggle TRY_DIRECT_SQL and assert no unexpected direct SQL errors. |
| standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/client/TestAlterPartitions.java | Updates expected exception types for rename-partition negative tests. |
| standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/VerifyingObjectStore.java | Refactors verification paths to use TableStore + config toggling rather than internal SQL/ORM helpers. |
| standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestObjectStore.java | Migrates tests to TableStore APIs and adds TRY_DIRECT_SQL toggling scopes. |
| standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetaStore.java | Removes unused imports. |
| standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/StatisticsTestUtils.java | Ensures ColumnStatisticsDesc includes catalog name. |
| standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/metastore/impl/PrivilegeStoreImpl.java | New privilege store implementation separated from RawStore. |
| standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/metastore/impl/NotificationStoreImpl.java | New notification store implementation separated from RawStore. |
| standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/metastore/iface/TableStore.java | New interface defining table/partition-related store APIs. |
| standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/metastore/iface/PrivilegeStore.java | New interface defining privilege/role store APIs. |
| standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/metastore/iface/NotificationStore.java | New interface defining notification event store APIs. |
| standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/metastore/TransactionHandler.java | Dynamic-proxy invocation handler that opens/commits/rolls back transactions and closes tracked queries. |
| standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/metastore/RawStoreAware.java | Base class to inject RawStore and PersistenceManager into store implementations. |
| standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/metastore/PersistenceManagerProxy.java | Proxy wrapper to track opened queries and expose execution context via an auxiliary interface. |
| standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/metastore/MetaDescriptor.java | New annotation used to discover store aliases and transactional behavior. |
| standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/metastore/GetListHelper.java | New helper specialization for list-returning GetHelper use cases. |
| standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/metastore/GetHelper.java | New transaction-aware SQL-vs-ORM helper with direct SQL fallback behavior. |
| standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/directsql/MetastoreDirectSqlUtils.java | Adjusts execution-context access to support proxied PersistenceManager. |
| standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/cache/CachedStore.java | Removes/adjusts delegations in favor of new store unwrapping and adds missing database model import. |
| standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/RawStore.java | Converts many APIs to default methods delegating to unwrap()ed stores; adds unwrap(Class<T>). |
| standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java | Moves partition-existence check earlier in alter-partition flow. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| case COLUMN: | ||
| Preconditions.checkArgument(objToRefresh.getColumnName()==null, "columnName must be null"); | ||
| grants = getTableAllColumnGrants(catName, objToRefresh.getDbName(), | ||
| objToRefresh.getObjectName(), authorizer); | ||
| break; |
There was a problem hiding this comment.
In the COLUMN case, this asserts objToRefresh.getColumnName() == null, but HiveObjectRef.columnName is a required thrift field and callers (e.g., PrivilegeHandler) pass an actual column name. This will cause refresh_privileges to fail for column objects. Also, the code fetches all column grants for the table; it should load only the relevant column/partition-column grants based on objToRefresh (columnName + optional partValues).
292f402 to
8a97567
Compare
6b82d3a to
34b344e
Compare
9d32149 to
5b00a98
Compare
5b00a98 to
ad7346d
Compare
|
|
cc @deniskuzZ @okumin @ngsg could you take a look if have time? thanks! |
ngsg
left a comment
There was a problem hiding this comment.
Hi @dengzhhu653, I started by looking at the overall direction of this PR and some of the test cases, and left a few comments.
My understanding is that this PR is a refactoring intended to reduce the complexity of ObjectStore. Overall, the direction looks reasonable to me. For example, separating transaction handling into TransactionHandler seems helpful for simplifying the logic in the sub-stores
That said, it would be helpful to have a related JIRA, if one exists, so one could better understand the motivation and proposed methods of this PR. I am also curious whether there has already been any discussion about refactoring ObjectStore.
| } catch (RuntimeException e) { | ||
| // A hack to verify that authorization check passed. Exception can be thrown be cause | ||
| // the functions are not being called with valid params. | ||
| // verify that exception has come from ObjectStore code, which means that the |
There was a problem hiding this comment.
I think the class name in the comment should also be updated.
| @@ -1,3 +1,4 @@ | |||
| --! qt:disabled:HIVE-25965 | |||
There was a problem hiding this comment.
I checked the Jenkins artifact and confirmed that the HIVE-25965 issue is being hit. However, I do not yet fully understand why this starts happening after applying this patch. Could you explain the expected connection between this patch and the failure?
Also, I may have missed something, but I could not reproduce the issue in my local environment. Were you able to reproduce it locally?



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?