Copilot commented on code in PR #6311:
URL: https://github.com/apache/hive/pull/6311#discussion_r2791918084
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java:
##########
@@ -4544,25 +4279,13 @@ public Partition
append_partition_by_name_with_environment_context(final String
final String tbl_name, final String part_name, final EnvironmentContext
env_context)
throws TException {
String[] parsedDbName = parseDbName(db_name, conf);
- Partition ret = null;
- Exception ex = null;
- try {
- AppendPartitionsRequest appendPartitionRequest = new
AppendPartitionsRequest();
- appendPartitionRequest.setDbName(parsedDbName[DB_NAME]);
- appendPartitionRequest.setTableName(tbl_name);
- appendPartitionRequest.setName(part_name);
- appendPartitionRequest.setCatalogName(parsedDbName[CAT_NAME]);
- appendPartitionRequest.setEnvironmentContext(env_context);
- ret = append_partition_req(appendPartitionRequest);
- } catch (Exception e) {
- ex = e;
- throw handleException(e)
- .throwIfInstance(InvalidObjectException.class,
AlreadyExistsException.class, MetaException.class)
- .defaultMetaException();
- } finally {
- endFunction("append_partition_by_name", ret != null, ex, tbl_name);
- }
- return ret;
+ AppendPartitionsRequest appendPartitionRequest = new
AppendPartitionsRequest();
+ appendPartitionRequest.setDbName(parsedDbName[DB_NAME]);
+ appendPartitionRequest.setTableName(tbl_name);
+ appendPartitionRequest.setName(part_name);
+ appendPartitionRequest.setCatalogName(parsedDbName[CAT_NAME]);
+ appendPartitionRequest.setEnvironmentContext(env_context);
+ return append_partition_req(appendPartitionRequest);
}
Review Comment:
append_partition_by_name_with_environment_context() no longer calls
startFunction/endFunction for the *append_partition_by_name* API name and
delegates directly to append_partition_req(). This changes what end-function
listeners/metrics report for callers using this deprecated API. Consider
re-adding start/endFunction instrumentation around the delegation so listeners
still observe the original API invocation name and outcome.
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java:
##########
@@ -4592,27 +4315,7 @@ public boolean
drop_partition_by_name_with_environment_context(final String db_n
public List<Partition> get_partitions_ps(final String db_name,
final String tbl_name, final
List<String> part_vals,
final short max_parts) throws
TException {
- String[] parsedDbName = parseDbName(db_name, conf);
- startPartitionFunction("get_partitions_ps", parsedDbName[CAT_NAME],
parsedDbName[DB_NAME], tbl_name, max_parts,
- part_vals);
-
- List<Partition> ret = null;
- Exception ex = null;
- try {
- authorizeTableForPartitionMetadata(parsedDbName[CAT_NAME],
parsedDbName[DB_NAME], tbl_name);
- // Don't send the parsedDbName, as this method will parse itself.
- ret = get_partitions_ps_with_auth(db_name, tbl_name, new
GetPartitionsArgs.GetPartitionsArgsBuilder()
- .part_vals(part_vals).max(max_parts)
- .build());
- ret = FilterUtils.filterPartitionsIfEnabled(isServerFilterEnabled,
filterHook, ret);
- } catch (Exception e) {
- ex = e;
- rethrowException(e);
- } finally {
- endFunction("get_partitions_ps", ret != null, ex, tbl_name);
- }
-
- return ret;
+ return get_partitions_ps_with_auth(db_name, tbl_name, part_vals,
max_parts, null, null);
}
Review Comment:
get_partitions_ps() now directly delegates to get_partitions_ps_with_auth()
without invoking startPartitionFunction/endFunction for the get_partitions_ps
API. This changes what MetaStoreEndFunctionListener/metrics observe for callers
of the deprecated API. Consider restoring the wrapper’s start/end function
instrumentation (or otherwise preserving the original API name in end-function
callbacks).
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java:
##########
@@ -2774,62 +2657,35 @@ public Partition
append_partition_with_environment_context(final String dbName,
final String tableName, final List<String> part_vals, final
EnvironmentContext envContext)
throws InvalidObjectException, AlreadyExistsException, MetaException {
String[] parsedDbName = parseDbName(dbName, conf);
- startPartitionFunction("append_partition_with_environment_context",
parsedDbName[CAT_NAME], parsedDbName[DB_NAME], tableName, part_vals);
- Partition ret = null;
- Exception ex = null;
- try {
- AppendPartitionsRequest appendPartitionsReq = new
AppendPartitionsRequest();
- appendPartitionsReq.setDbName(parsedDbName[DB_NAME]);
- appendPartitionsReq.setTableName(tableName);
- appendPartitionsReq.setPartVals(part_vals);
- appendPartitionsReq.setCatalogName(parsedDbName[CAT_NAME]);
- appendPartitionsReq.setEnvironmentContext(envContext);
- ret = append_partition_req(appendPartitionsReq);
- } catch (Exception e) {
- ex = e;
- throw handleException(e).throwIfInstance(MetaException.class,
InvalidObjectException.class, AlreadyExistsException.class)
- .defaultMetaException();
- } finally {
- endFunction("append_partition_with_environment_context", ret != null,
ex, tableName);
- }
- return ret;
+ AppendPartitionsRequest appendPartitionsReq = new
AppendPartitionsRequest();
+ appendPartitionsReq.setDbName(parsedDbName[DB_NAME]);
+ appendPartitionsReq.setTableName(tableName);
+ appendPartitionsReq.setPartVals(part_vals);
+ appendPartitionsReq.setCatalogName(parsedDbName[CAT_NAME]);
+ appendPartitionsReq.setEnvironmentContext(envContext);
+ return append_partition_req(appendPartitionsReq);
}
Review Comment:
append_partition_with_environment_context() no longer wraps the operation
with startPartitionFunction/endFunction for the
*append_partition_with_environment_context* API name, and instead relies on
append_partition_req()'s logging/listener callbacks. This changes the function
name and input details observed by MetaStoreEndFunctionListener/metrics for
callers using the deprecated API. Consider restoring start/endFunction
instrumentation in this wrapper (or otherwise ensuring end-function listeners
see the original API name and context).
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/handler/GetPartitionsHandler.java:
##########
@@ -0,0 +1,576 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hive.metastore.handler;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.LinkedList;
+import java.util.List;
+import java.util.function.Consumer;
+
+import org.apache.commons.lang3.tuple.Pair;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.common.StatsSetupConst;
+import org.apache.hadoop.hive.common.TableName;
+import org.apache.hadoop.hive.metastore.HMSHandler;
+import org.apache.hadoop.hive.metastore.IHMSHandler;
+import org.apache.hadoop.hive.metastore.MetaStoreFilterHook;
+import org.apache.hadoop.hive.metastore.RawStore;
+import org.apache.hadoop.hive.metastore.Warehouse;
+import org.apache.hadoop.hive.metastore.api.ColumnStatistics;
+import org.apache.hadoop.hive.metastore.api.FieldSchema;
+import org.apache.hadoop.hive.metastore.api.GetTableRequest;
+import org.apache.hadoop.hive.metastore.api.MetaException;
+import org.apache.hadoop.hive.metastore.api.NoSuchObjectException;
+import org.apache.hadoop.hive.metastore.api.Partition;
+import org.apache.hadoop.hive.metastore.api.PartitionValuesResponse;
+import org.apache.hadoop.hive.metastore.api.Table;
+import org.apache.hadoop.hive.metastore.client.builder.GetPartitionsArgs;
+import org.apache.hadoop.hive.metastore.conf.MetastoreConf;
+import org.apache.hadoop.hive.metastore.events.PreReadTableEvent;
+import org.apache.hadoop.hive.metastore.utils.FilterUtils;
+import org.apache.thrift.TException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import static
org.apache.hadoop.hive.metastore.ExceptionHandler.handleException;
+import static
org.apache.hadoop.hive.metastore.HMSHandler.PARTITION_NUMBER_EXCEED_LIMIT_MSG;
+import static
org.apache.hadoop.hive.metastore.utils.StringUtils.normalizeIdentifier;
+
+// Collect get partitions APIs together
+@SuppressWarnings({"unchecked", "rawtypes"})
+@RequestHandler(requestBody = GetPartitionsHandler.GetPartitionsRequest.class)
+public class GetPartitionsHandler<T> extends
AbstractRequestHandler<GetPartitionsHandler.GetPartitionsRequest,
+ GetPartitionsHandler.GetPartitionsResult<T>> {
+ private static final Logger LOG =
LoggerFactory.getLogger(GetPartitionsHandler.class);
+ private static final String NO_FILTER_STRING = "";
+ private RawStore rs;
+ private String catName;
+ private String dbName;
+ private String tblName;
+ private GetPartitionsArgs args;
+ private Table table;
+ private Configuration conf;
+ private GetPartitionsMethod getMethod;
+ private MetaStoreFilterHook filterHook;
+ private boolean isServerFilterEnabled;
+
+ enum GetPartitionsMethod {
+ EXPR, NAMES, FILTER, PART_VALS, ALL, VALUES
+ }
+
+ GetPartitionsHandler(IHMSHandler handler, GetPartitionsRequest request) {
+ super(handler, false, request);
+ }
+
+ @Override
+ protected void beforeExecute() throws TException, IOException {
+ args = request.getGetPartitionsArgs();
+ if (request.isGetPartitionValues()) {
+ getMethod = GetPartitionsMethod.VALUES;
+ } else if (args.getExpr() != null) {
+ getMethod = GetPartitionsMethod.EXPR;
+ } else if (args.getFilter() != null) {
+ getMethod = GetPartitionsMethod.FILTER;
+ } else if (args.getPartNames() != null) {
+ getMethod = GetPartitionsMethod.NAMES;
+ } else if (args.getPart_vals() != null) {
+ getMethod = GetPartitionsMethod.PART_VALS;
+ } else {
+ getMethod = GetPartitionsMethod.ALL;
+ }
+
+ catName = normalizeIdentifier(request.getTableName().getCat());
+ dbName = normalizeIdentifier(request.getTableName().getDb());
+ tblName = normalizeIdentifier(request.getTableName().getTable());
+ conf = handler.getConf();
+ rs = handler.getMS();
+ filterHook = handler.getMetaFilterHook();
+ isServerFilterEnabled = filterHook != null;
+ GetTableRequest getTableRequest = new GetTableRequest(dbName, tblName);
+ getTableRequest.setCatName(catName);
+ table = handler.get_table_core(getTableRequest);
+ ((HMSHandler) handler).firePreEvent(new PreReadTableEvent(table, handler));
+ authorizeTableForPartitionMetadata();
+
+ LOG.info("Starting to get {} of {} using {}", request.isFetchPartNames() ?
"partition names" : "partitions",
+ TableName.getQualified(catName, dbName, tblName), getMethod);
+ }
+
+ @Override
+ protected GetPartitionsResult<T> execute() throws TException, IOException {
+ return (GetPartitionsResult<T>) switch (getMethod) {
+ case EXPR -> getPartitionsByExpr();
+ case FILTER -> getPartitionsByFilter();
+ case NAMES -> getPartitionsByNames();
+ case PART_VALS -> getPartitionsByVals();
+ case ALL -> getPartitions();
+ case VALUES -> getPartitionValues();
+ };
+ }
+
+ private GetPartitionsResult getPartitionsByVals() throws TException {
+ if (request.isFetchPartNames()) {
+ List<String> ret = rs.listPartitionNamesPs(catName, dbName, tblName,
+ args.getPart_vals(), (short) args.getMax());
+ return new GetPartitionsResult<>(ret, true);
+ } else {
+ List<Partition> ret;
+ if (args.getPart_vals() != null) {
+ checkLimitNumberOfPartitionsByPs(args.getPart_vals(), args.getMax());
+ } else {
+ checkLimitNumberOfPartitionsByFilter(NO_FILTER_STRING, args.getMax());
+ }
+ ret = rs.listPartitionsPsWithAuth(catName, dbName, tblName, args);
+ return new GetPartitionsResult(ret, true);
+ }
+ }
+
+ private GetPartitionsResult getPartitionValues() throws MetaException {
+ PartitionValuesResponse resp = rs.listPartitionValues(catName, dbName,
tblName, request.getPartitionKeys(),
+ request.isApplyDistinct(), args.getFilter(), request.isAscending(),
+ request.getPartitionOrders(), args.getMax());
+ return new GetPartitionsResult<>(List.of(resp), true);
+ }
+
+ private void checkLimitNumberOfPartitionsByPs(List<String> partVals, int
requestMax) throws TException {
+ if (exceedsPartitionFetchLimit(requestMax)) {
+ checkLimitNumberOfPartitions(tblName, rs.getNumPartitionsByPs(catName,
dbName, tblName, partVals));
+ }
+ }
+
+ private GetPartitionsResult<Partition> getPartitionsByFilter() throws
TException {
+ List<Partition> ret = null;
+ if (exceedsPartitionFetchLimit(args.getMax())) {
+ // Since partition limit is configured, we need fetch at most (limit +
1) partition names
+ int max = MetastoreConf.getIntVar(conf,
MetastoreConf.ConfVars.LIMIT_PARTITION_REQUEST) + 1;
+ args = new
GetPartitionsArgs.GetPartitionsArgsBuilder(args).max(max).build();
+ List<String> partNames = rs.listPartitionNamesByFilter(catName, dbName,
tblName, args);
+ checkLimitNumberOfPartitions(tblName, partNames.size());
+ ret = rs.getPartitionsByNames(catName, dbName, tblName,
+ new
GetPartitionsArgs.GetPartitionsArgsBuilder(args).partNames(partNames).build());
+ } else {
+ ret = rs.getPartitionsByFilter(catName, dbName, tblName, args);
+ }
+
+ return new GetPartitionsResult<>(ret, true);
+ }
+
+ /**
+ * Check if user can access the table associated with the partition. If not,
then throw exception
+ * so user cannot access partitions associated with this table
+ * We are not calling Pre event listener for authorization because it
requires getting the
+ * table object from DB, more overhead. Instead ,we call filter hook to
filter out table if user
+ * has no access. Filter hook only requires table name, not table object.
That saves DB access for
+ * table object, and still achieve the same purpose: checking if user can
access the specified
+ * table
+ *
+ * @throws NoSuchObjectException
+ * @throws MetaException
+ */
+ private void authorizeTableForPartitionMetadata() throws
NoSuchObjectException, MetaException {
+ FilterUtils.checkDbAndTableFilters(
+ isServerFilterEnabled, filterHook, catName, dbName, tblName);
+ }
Review Comment:
The Javadoc on authorizeTableForPartitionMetadata() says the handler avoids
fetching the table object and avoids firing pre-event listeners for
authorization. However, beforeExecute() now unconditionally loads the Table via
get_table_core() and fires a PreReadTableEvent before calling this method, so
the comment is no longer accurate. Update the Javadoc to match the current
behavior (or adjust the flow if the intent is still to avoid that overhead for
some paths).
--
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]
