Copilot commented on code in PR #6427:
URL: https://github.com/apache/hive/pull/6427#discussion_r3070731179
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java:
##########
@@ -1626,185 +1483,39 @@ public Table get_table_core(GetTableRequest
getTableRequest) throws MetaExceptio
* @throws InvalidOperationException
* @throws UnknownDBException
*/
-
@Override
public GetTablesResult get_table_objects_by_name_req(GetTablesRequest req)
throws TException {
- String catName = req.isSetCatName() ? req.getCatName() :
getDefaultCatalog(conf);
- if (isDatabaseRemote(req.getDbName())) {
- return new
GetTablesResult(getRemoteTableObjectsInternal(req.getDbName(),
req.getTblNames(), req.getTablesPattern()));
- }
- return new GetTablesResult(getTableObjectsInternal(catName,
req.getDbName(),
- req.getTblNames(), req.getCapabilities(), req.getProjectionSpec(),
req.getTablesPattern()));
- }
-
- private List<Table> filterTablesByName(List<Table> tables, List<String>
tableNames) {
- List<Table> filteredTables = new ArrayList<>();
- for (Table table : tables) {
- if (tableNames.contains(table.getTableName())) {
- filteredTables.add(table);
- }
- }
- return filteredTables;
- }
-
- private List<Table> getRemoteTableObjectsInternal(String dbname,
List<String> tableNames, String pattern) throws MetaException {
- String[] parsedDbName = parseDbName(dbname, conf);
- try {
- // retrieve tables from remote database
- Database db = get_database_core(parsedDbName[CAT_NAME],
parsedDbName[DB_NAME]);
- List<Table> tables =
DataConnectorProviderFactory.getDataConnectorProvider(db).getTables(null);
-
- // filtered out undesired tables
- if (tableNames != null) {
- tables = filterTablesByName(tables, tableNames);
- }
-
- // set remote tables' local hive database reference
- for (Table table : tables) {
- table.setDbName(dbname);
- }
-
- return FilterUtils.filterTablesIfEnabled(isServerFilterEnabled,
filterHook, tables);
- } catch (Exception e) {
- LOG.warn("Unexpected exception while getting table(s) in remote database
" + dbname , e);
- if (isInTest) {
- // ignore the exception
- return new ArrayList<Table>();
- } else {
- throw newMetaException(e);
- }
- }
- }
-
- private List<Table> getTableObjectsInternal(String catName, String dbName,
- List<String> tableNames,
- ClientCapabilities capabilities,
- GetProjectionsSpec
projectionsSpec, String tablePattern)
- throws MetaException, InvalidOperationException, UnknownDBException {
- if (isInTest) {
- assertClientHasCapability(capabilities, ClientCapability.TEST_CAPABILITY,
- "Hive tests", "get_table_objects_by_name_req");
- }
-
- if (projectionsSpec != null) {
- if (!projectionsSpec.isSetFieldList() &&
(projectionsSpec.isSetIncludeParamKeyPattern() ||
- projectionsSpec.isSetExcludeParamKeyPattern())) {
- throw new InvalidOperationException("Include and Exclude Param key are
not supported.");
- }
- }
-
- List<Table> tables = new ArrayList<>();
- startMultiTableFunction("get_multi_table", dbName, tableNames);
- Exception ex = null;
- int tableBatchSize = MetastoreConf.getIntVar(conf,
- ConfVars.BATCH_RETRIEVE_MAX);
-
try {
- if (dbName == null || dbName.isEmpty()) {
- throw new UnknownDBException("DB name is null or empty");
- }
- RawStore ms = getMS();
- if(tablePattern != null){
- tables = ms.getTableObjectsByName(catName, dbName, tableNames,
projectionsSpec, tablePattern);
- }else {
- if (tableNames == null) {
- throw new InvalidOperationException(dbName + " cannot find null
tables");
- }
-
- // The list of table names could contain duplicates.
RawStore.getTableObjectsByName()
- // only guarantees returning no duplicate table objects in one batch.
If we need
- // to break into multiple batches, remove duplicates first.
- List<String> distinctTableNames = tableNames;
- if (distinctTableNames.size() > tableBatchSize) {
- List<String> lowercaseTableNames = new ArrayList<>();
- for (String tableName : tableNames) {
- lowercaseTableNames.add(normalizeIdentifier(tableName));
- }
- distinctTableNames = new ArrayList<>(new
HashSet<>(lowercaseTableNames));
- }
-
- int startIndex = 0;
- // Retrieve the tables from the metastore in batches. Some databases
like
- // Oracle cannot have over 1000 expressions in a in-list
- while (startIndex < distinctTableNames.size()) {
- int endIndex = Math.min(startIndex + tableBatchSize,
distinctTableNames.size());
- tables.addAll(ms.getTableObjectsByName(catName, dbName,
distinctTableNames.subList(
- startIndex, endIndex), projectionsSpec, tablePattern));
- startIndex = endIndex;
- }
- }
- for (Table t : tables) {
- if (t.getParameters() != null &&
MetaStoreUtils.isInsertOnlyTableParam(t.getParameters())) {
- assertClientHasCapability(capabilities,
ClientCapability.INSERT_ONLY_TABLES,
- "insert-only tables", "get_table_req");
- }
- }
-
- tables = FilterUtils.filterTablesIfEnabled(isServerFilterEnabled,
filterHook, tables);
+ List<Table> tables =
+ GetTableHandler.getTables(() ->
startMultiTableFunction("get_multi_table", req.getDbName(), req.getTblNames()),
+ this, req,
+ t -> endFunction("get_multi_table", t.getLeft() != null,
t.getRight(), join(req.getTblNames(), ",")));
+ return new GetTablesResult(tables);
} catch (Exception e) {
- ex = e;
throw handleException(e)
.throwIfInstance(MetaException.class,
InvalidOperationException.class, UnknownDBException.class)
.defaultMetaException();
- } finally {
- endFunction("get_multi_table", tables != null, ex, join(tableNames,
","));
}
- return tables;
}
@Override
public void update_creation_metadata(String catName, final String dbName,
final String tableName, CreationMetadata cm) throws MetaException {
getMS().updateCreationMetadata(catName, dbName, tableName, cm);
}
Review Comment:
The local variable type `GetTableHandler<GetTableRequest, Table>` is
parameterized with `GetTableRequest`, but the actual request being
wrapped/offered here is a `GetTablesRequest`. This compiles via raw/unchecked
casts but makes the code harder to follow and can mask type errors. Prefer
`GetTableHandler<GetTablesRequest, Table>` (or a wildcard/raw type) to reflect
the actual request type being handled.
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java:
##########
@@ -1421,128 +1417,27 @@ public TruncateTableResponse
truncate_table_req(TruncateTableRequest req)
@Override
public List<ExtendedTableInfo> get_tables_ext(final GetTablesExtRequest req)
throws MetaException {
- List<String> tables = new ArrayList<String>();
- List<ExtendedTableInfo> ret = new ArrayList<ExtendedTableInfo>();
String catalog = req.getCatalog();
String database = req.getDatabase();
String pattern = req.getTableNamePattern();
- List<String> processorCapabilities = req.getProcessorCapabilities();
- int limit = req.getLimit();
- String processorId = req.getProcessorIdentifier();
- List<Table> tObjects = new ArrayList<>();
-
- startTableFunction("get_tables_ext", catalog, database, pattern);
- Exception ex = null;
try {
- tables = getMS().getTables(catalog, database, pattern, null, limit);
- LOG.debug("get_tables_ext:getTables() returned " + tables.size());
- tables = FilterUtils.filterTableNamesIfEnabled(isServerFilterEnabled,
filterHook,
- catalog, database, tables);
-
- if (tables.size() > 0) {
- tObjects = getMS().getTableObjectsByName(catalog, database, tables);
- LOG.debug("get_tables_ext:getTableObjectsByName() returned " +
tObjects.size());
- if (processorCapabilities == null || processorCapabilities.size() == 0
||
- processorCapabilities.contains("MANAGERAWMETADATA")) {
- LOG.info("Skipping translation for processor with " + processorId);
- } else {
- if (transformer != null) {
- Map<Table, List<String>> retMap = transformer.transform(tObjects,
processorCapabilities, processorId);
-
- for (Map.Entry<Table, List<String>> entry : retMap.entrySet()) {
- LOG.debug("Table " + entry.getKey().getTableName() + " requires
" + Arrays.toString((entry.getValue()).toArray()));
- ret.add(convertTableToExtendedTable(entry.getKey(),
entry.getValue(), req.getRequestedFields()));
- }
- } else {
- for (Table table : tObjects) {
- ret.add(convertTableToExtendedTable(table,
processorCapabilities, req.getRequestedFields()));
- }
- }
- }
- }
+ return
+ GetTableHandler.getTables(() -> startTableFunction("get_tables_ext",
catalog, database, pattern),
Review Comment:
`get_tables_ext` calls `startTableFunction` with `catalog =
req.getCatalog()` without defaulting when the request doesn't set a catalog.
`startTableFunction` calls `TableName.getQualified(catName, ...)`, which can
throw if `catName` is null, and it also makes the logged catalog differ from
the handler's behavior (which defaults catalog). Use `req.isSetCatalog() ?
req.getCatalog() : getDefaultCatalog(conf)` for the start/end logging context
to match actual execution.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
