Copilot commented on code in PR #6311:
URL: https://github.com/apache/hive/pull/6311#discussion_r2785460687
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java:
##########
@@ -3691,51 +3448,62 @@ public List<String> get_partition_names(final String
db_name, final String tbl_n
}
@Override
- public List<String> fetch_partition_names_req(final PartitionsRequest
partitionReq)
+ public List<String> fetch_partition_names_req(final PartitionsRequest req)
throws NoSuchObjectException, MetaException {
- String catName = partitionReq.getCatName();
- String dbName = partitionReq.getDbName();
- String tbl_name = partitionReq.getTblName();
- startTableFunction("fetch_partition_names_req", catName, dbName, tbl_name);
- fireReadTablePreEvent(catName, dbName, tbl_name);
- List<String> ret = null;
- Exception ex = null;
- try {
- authorizeTableForPartitionMetadata(catName, dbName, tbl_name);
- ret = getMS().listPartitionNames(catName, dbName, tbl_name,
partitionReq.getMaxParts());
- ret = FilterUtils.filterPartitionNamesIfEnabled(isServerFilterEnabled,
- filterHook, catName, dbName, tbl_name, ret);
- } catch (Exception e) {
- ex = e;
- throw newMetaException(e);
- } finally {
- endFunction("fetch_partition_names_req", ret != null, ex, tbl_name);
+ String catName = req.isSetCatName() ? req.getCatName() :
getDefaultCatalog(conf);
+ String dbName = req.getDbName(), tblName = req.getTblName();
+ TableName tableName = new TableName(catName, dbName, tblName);
+ try {
+ return GetPartitionsHandler.getPartitionNames(
+ t -> startTableFunction("fetch_partition_names_req", catName,
dbName, tblName),
+ ex -> endFunction("fetch_partition_names_req", ex == null, ex,
tableName.toString()), this, tableName,
+ new
GetPartitionsArgs.GetPartitionsArgsBuilder().max(req.getMaxParts()).build()).result();
+ } catch (TException ex) {
+ if (ex instanceof NoSuchObjectException e) {
+ // Keep it here just because some tests in TestListPartitions assume
NoSuchObjectException
+ // if the input is invalid.
+ if (StringUtils.isBlank(dbName) ||
StringUtils.isBlank(tableName.getTable())) {
+ throw e;
+ }
+ return Collections.emptyList();
+ }
+ throw handleException(ex).defaultMetaException();
}
- return ret;
}
@Override
public PartitionValuesResponse get_partition_values(PartitionValuesRequest
request)
throws MetaException {
String catName = request.isSetCatName() ? request.getCatName() :
getDefaultCatalog(conf);
- String dbName = request.getDbName();
- String tblName = request.getTblName();
+ TableName tableName = new TableName(catName, request.getDbName(),
request.getTblName());
long maxParts = request.getMaxParts();
String filter = request.isSetFilter() ? request.getFilter() : "";
- startPartitionFunction("get_partition_values", catName, dbName, tblName,
(int) maxParts, filter);
+ GetPartitionsHandler.GetPartitionsRequest getPartitionsRequest =
+ new GetPartitionsHandler.GetPartitionsRequest(tableName,
+ new GetPartitionsArgs.GetPartitionsArgsBuilder().max((int)
maxParts).filter(filter).build());
+ startPartitionFunction("get_partition_values", catName, tableName.getDb(),
tableName.getTable(),
+ (int) maxParts, filter);
+ Exception ex = null;
try {
- authorizeTableForPartitionMetadata(catName, dbName, tblName);
-
// This is serious black magic, as the following 2 lines do nothing
AFAICT but without them
// the subsequent call to listPartitionValues fails.
- List<FieldSchema> partCols = new ArrayList<FieldSchema>();
- partCols.add(request.getPartitionKeys().get(0));
- return getMS().listPartitionValues(catName, dbName, tblName,
request.getPartitionKeys(),
- request.isApplyDistinct(), request.getFilter(),
request.isAscending(),
- request.getPartitionOrder(), request.getMaxParts());
- } catch (NoSuchObjectException e) {
- LOG.error(String.format("Unable to get partition for %s.%s.%s", catName,
dbName, tblName), e);
- throw new MetaException(e.getMessage());
+ getPartitionsRequest.setApplyDistinct(request.isApplyDistinct());
+ getPartitionsRequest.setAscending(request.isAscending());
+ getPartitionsRequest.setPartitionKeys(request.getPartitionKeys());
+ getPartitionsRequest.setPartitionOrders(request.getPartitionOrder());
Review Comment:
The comment says “the following 2 lines do nothing … but without them the
subsequent call to listPartitionValues fails”, but the code no longer has those
two lines (and now sets multiple fields on GetPartitionsRequest instead).
Please update or remove this comment so it accurately reflects the current
required workaround and what actually fixes the failure.
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java:
##########
@@ -3313,37 +3169,18 @@ public Partition get_partition(final String db_name,
final String tbl_name,
return ret;
}
- private Partition get_partition_core(final String db_name, final String
tbl_name,
- final List<String> part_vals) throws
MetaException, NoSuchObjectException {
- String[] parsedDbName = parseDbName(db_name, conf);
- startPartitionFunction("get_partition_core", parsedDbName[CAT_NAME],
parsedDbName[DB_NAME],
- tbl_name, part_vals);
-
- Partition ret = null;
- Exception ex = null;
- try {
- authorizeTableForPartitionMetadata(parsedDbName[CAT_NAME],
parsedDbName[DB_NAME], tbl_name);
- fireReadTablePreEvent(parsedDbName[CAT_NAME], parsedDbName[DB_NAME],
tbl_name);
- ret = getMS().getPartition(parsedDbName[CAT_NAME],
parsedDbName[DB_NAME], tbl_name, part_vals);
- ret = FilterUtils.filterPartitionIfEnabled(isServerFilterEnabled,
filterHook, ret);
- } catch (Exception e) {
- ex = e;
- throw handleException(e).throwIfInstance(MetaException.class,
NoSuchObjectException.class).defaultMetaException();
- } finally {
- endFunction("get_partition_core", ret != null, ex, tbl_name);
- }
- return ret;
- }
-
@Override
public GetPartitionResponse get_partition_req(GetPartitionRequest req)
throws MetaException, NoSuchObjectException, TException {
- // TODO Move the logic from get_partition to here, as that method is
getting deprecated
- String dbName = MetaStoreUtils.prependCatalogToDbName(req.getCatName(),
req.getDbName(), conf);
- Partition p = get_partition_core(dbName, req.getTblName(),
req.getPartVals());
- GetPartitionResponse res = new GetPartitionResponse();
- res.setPartition(p);
- return res;
+ String catName = req.isSetCatName() ? req.getCatName() :
getDefaultCatalog(conf);
+ TableName tableName = new TableName(catName, req.getDbName(),
req.getTblName());
+ GetPartitionsHandler.validatePartVals(this, tableName, req.getPartVals());
+ List<Partition> partitions = GetPartitionsHandler.getPartitions(
+ t -> startTableFunction("get_partition_req", catName, t.getDb(),
t.getCat()),
+ ex -> endFunction("get_partition_req", ex == null, ex,
tableName.toString()),
Review Comment:
In get_partition_req(), the startTableFunction lambda passes `t.getCat()` as
the table name argument (and `t.getDb()` as db). This logs/records the wrong
qualified table for metrics/end-function listeners. It should pass the actual
table name (e.g., `t.getTable()`), and generally use
`t.getCat()`/`t.getDb()`/`t.getTable()` consistently rather than mixing
`catName` and `t` fields.
```suggestion
t -> startTableFunction("get_partition_req", t.getCat(), t.getDb(),
t.getTable()),
ex -> endFunction("get_partition_req", ex == null, ex,
req.getTblName()),
```
##########
standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetaStoreMethods.java:
##########
@@ -21,6 +21,7 @@
import org.apache.hadoop.conf.Configuration;
import org.apache.hadoop.hive.metastore.annotation.MetastoreCheckinTest;
import org.apache.hadoop.hive.metastore.api.InvalidObjectException;
Review Comment:
`InvalidObjectException` is still imported but no longer referenced in this
test after switching the expected exception to `NoSuchObjectException`. If the
project enforces no-unused-imports (e.g., via checkstyle), this will fail the
build; please remove the unused import.
```suggestion
```
--
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]
