HIVE-29452: Split get table from HMSHandler#6427
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors Hive Metastore “get table”-related logic by moving several table retrieval/listing flows out of HMSHandler into a dedicated GetTableHandler request handler, aligning with the existing request-handler architecture used elsewhere in the standalone metastore server.
Changes:
- Introduces
GetTableHandlerto centralize implementations forget_table_req,get_table_core,get_table_objects_by_name_req, and various table-name listing methods. - Updates
HMSHandlerto delegate these operations toGetTableHandlerand removes now-duplicated helper logic. - Improves an
UnsupportedOperationExceptionmessage inGetPartitionsHandlerto include the unsupported request object.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java |
Delegates multiple table APIs to the new GetTableHandler and removes inlined implementations. |
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/handler/GetTableHandler.java |
New request handler consolidating table fetch/list logic previously in HMSHandler. |
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/handler/GetPartitionsHandler.java |
Makes the “not yet implemented” exception include the request value for easier debugging. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java:1449
- In
get_table_req, exceptions fromGetTableHandler.getTable(...)aren't captured/passed toendFunction(theExceptionargument is alwaysnull). This can hide failures from end-function listeners/metrics and produces misleading audit/telemetry. Capture the exception in a localException ex(as other HMSHandler methods do), rethrow it, and passextoendFunction.
@Override
public List<TableMeta> get_table_meta(String dbnames, String tblNames, List<String> tblTypes)
throws MetaException, NoSuchObjectException {
List<TableMeta> t = null;
String[] parsedDbName = parseDbName(dbnames, conf);
startTableFunction("get_table_metas", parsedDbName[CAT_NAME], parsedDbName[DB_NAME], tblNames);
Exception ex = null;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
wecharyu
left a comment
There was a problem hiding this comment.
Looks good overall, just left a few minor comments.
| } | ||
| return t; | ||
| return GetTableHandler.getTables( | ||
| () -> startTableFunction("get_table_metas", parsedDbName[CAT_NAME], parsedDbName[DB_NAME], tblNames), |
There was a problem hiding this comment.
nit: get_table_metas => get_table_meta
There was a problem hiding this comment.
metastore metrics will use this field, changing it to a new one might break the old graph we built for this method
| } | ||
| return tables; | ||
| List<Table> tables = GetTableHandler.getTables( | ||
| () -> startMultiTableFunction("get_multi_table", req.getDbName(), req.getTblNames()), |
There was a problem hiding this comment.
nit: get_multi_table => get_table_objects
| try { | ||
| Database database = handler.get_database_core(catName, dbname); | ||
| if (isDatabaseRemote(database)) { | ||
| return DataConnectorProviderFactory.getDataConnectorProvider(database).getTableNames(); |
There was a problem hiding this comment.
It does not filter out the result by possible filter or pattern.
There was a problem hiding this comment.
good catch, we ignore the filter/pattern for remote database before the change, maybe we need to address it in the future.
added the missing authorization filter.
|
|
Thank you for the review @wecharyu! |



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?