Copilot commented on code in PR #6935:
URL: https://github.com/apache/hive/pull/6935#discussion_r4520654590
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/handler/TruncateTableHandler.java:
##########
@@ -142,112 +124,162 @@ protected TruncateTableResult execute() throws
TException, IOException {
.map(Boolean::parseBoolean)
.orElse(false);
- if (TxnUtils.isTransactionalTable(table) || !skipDataDeletion) {
- if (!skipDataDeletion) {
- isSkipTrash = MetaStoreUtils.isSkipTrash(table.getParameters());
- Database db = handler.get_database_core(catName, dbName);
- needCmRecycle = ReplChangeManager.shouldEnableCm(db, table);
- }
- List<Path> locations = new ArrayList<>();
- if (0 != table.getPartitionKeysSize()) {
- for (Partition partition : partitions) {
- locations.add(new Path(partition.getSd().getLocation()));
- }
- } else {
- locations.add(new Path(table.getSd().getLocation()));
- }
- // This is not transactional
- for (Path location : locations) {
- if (!skipDataDeletion) {
- truncateDataFiles(location, isSkipTrash, needCmRecycle);
- } else {
- // For Acid tables we don't need to delete the old files, only write
an empty baseDir.
- // Compaction and cleaner will take care of the rest
- addTruncateBaseFile(location, request.getWriteId(),
- handler.getConf(), AcidMetaDataFile.DataFormat.TRUNCATED);
- }
- }
+ if (!skipDataDeletion) {
+ isSkipTrash = MetaStoreUtils.isSkipTrash(table.getParameters());
+ Database db = handler.get_database_core(catName, dbName);
+ needCmRecycle = ReplChangeManager.shouldEnableCm(db, table);
+ }
+
+ if (skipDataDeletion && !isTxnTable) {
+ throw new MetaException("Cannot truncate the table as request configured
to skip deleting data" +
+ " and the target isn't a transactional table");
Review Comment:
`HiveIcebergMetaHook.preTruncateTable()` commits the Iceberg delete and sets
`truncateSkipDataDeletion=true`, but Iceberg tables are not Hive transactional
tables. This guard therefore reports failure after the delete has committed and
skips the HMS statistics reset. Remove the guard to preserve metadata-only
truncation; `truncateLocation()` already avoids creating an ACID base directory
for non-transactional tables.
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/handler/TruncateTableHandler.java:
##########
@@ -142,112 +124,162 @@ protected TruncateTableResult execute() throws
TException, IOException {
.map(Boolean::parseBoolean)
.orElse(false);
- if (TxnUtils.isTransactionalTable(table) || !skipDataDeletion) {
- if (!skipDataDeletion) {
- isSkipTrash = MetaStoreUtils.isSkipTrash(table.getParameters());
- Database db = handler.get_database_core(catName, dbName);
- needCmRecycle = ReplChangeManager.shouldEnableCm(db, table);
- }
- List<Path> locations = new ArrayList<>();
- if (0 != table.getPartitionKeysSize()) {
- for (Partition partition : partitions) {
- locations.add(new Path(partition.getSd().getLocation()));
- }
- } else {
- locations.add(new Path(table.getSd().getLocation()));
- }
- // This is not transactional
- for (Path location : locations) {
- if (!skipDataDeletion) {
- truncateDataFiles(location, isSkipTrash, needCmRecycle);
- } else {
- // For Acid tables we don't need to delete the old files, only write
an empty baseDir.
- // Compaction and cleaner will take care of the rest
- addTruncateBaseFile(location, request.getWriteId(),
- handler.getConf(), AcidMetaDataFile.DataFormat.TRUNCATED);
- }
- }
+ if (!skipDataDeletion) {
+ isSkipTrash = MetaStoreUtils.isSkipTrash(table.getParameters());
+ Database db = handler.get_database_core(catName, dbName);
+ needCmRecycle = ReplChangeManager.shouldEnableCm(db, table);
+ }
+
+ if (skipDataDeletion && !isTxnTable) {
+ throw new MetaException("Cannot truncate the table as request configured
to skip deleting data" +
+ " and the target isn't a transactional table");
}
- // Alter the table/partition stats and also notify truncate table event
+
boolean success;
if (isPartitioned) {
- success = alterPartitionsForTruncate();
+ success = truncatePartitions(isSkipTrash, needCmRecycle,
skipDataDeletion);
} else {
+ truncateLocation(new Path(table.getSd().getLocation()), isSkipTrash,
needCmRecycle, skipDataDeletion);
success = alterTableStatsForTruncate();
}
return new TruncateTableResult(success);
}
- private void updateStatsForTruncate(Map<String,String> props,
EnvironmentContext environmentContext) {
- if (null == props) {
- return;
+ private PartitionsIterable newPartitionBatches(int batchSize) throws
MetaException {
+ TableName qualifiedTable = new TableName(catName, dbName,
request.getTableName());
+ if (reqPartNames != null) {
+ return PartitionsIterable.create(
+ ms.unwrap(TableStore.class), qualifiedTable, batchSize,
reqPartNames);
}
- for (String stat : StatsSetupConst.SUPPORTED_STATS) {
- String statVal = props.get(stat);
- if (statVal != null) {
- //In the case of truncate table, we set the stats to be 0.
- props.put(stat, "0");
+ return PartitionsIterable.create(ms, qualifiedTable, batchSize);
+ }
+
+ private boolean truncatePartitions(boolean isSkipTrash, boolean
needCmRecycle, boolean skipDataDeletion)
+ throws TException, IOException {
+ if (reqPartNames != null && reqPartNames.isEmpty()) {
+ return true;
+ }
+ int batchSize = MetastoreConf.getIntVar(handler.getConf(),
+ MetastoreConf.ConfVars.BATCH_RETRIEVE_MAX);
+ PartitionsIterable partitionBatches = newPartitionBatches(batchSize);
+
+ boolean success = true;
+ for (List<Partition> partitions : partitionBatches) {
+ if (partitions.isEmpty()) {
+ continue;
+ }
+ EnvironmentContext environmentContext = new EnvironmentContext();
+ List<Partition> newPartitions = buildAndCheckPartitions(partitions,
environmentContext);
Review Comment:
`buildAndCheckPartitions()` fires `PreAlterPartitionEvent`, so a listener
rejection in a later batch now fails the request after earlier batches have had
their files deleted and metadata committed. Previously all partition pre-events
ran before any deletion. Validate all targeted partitions in a bounded-memory
pass before starting destructive work, without firing stateful pre-events twice.
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/metastore/PartitionsIterable.java:
##########
@@ -0,0 +1,150 @@
+/*
+ * 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.metastore;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.Iterator;
+import java.util.List;
+import java.util.NoSuchElementException;
+import java.util.Objects;
+
+import org.apache.hadoop.hive.common.TableName;
+import org.apache.hadoop.hive.metastore.RawStore;
+import org.apache.hadoop.hive.metastore.api.MetaException;
+import org.apache.hadoop.hive.metastore.api.Partition;
+import org.apache.hadoop.hive.metastore.client.builder.GetPartitionsArgs;
+import org.apache.hadoop.hive.metastore.metastore.iface.TableStore;
+
+/**
+ * PartitionsIterable - effectively a lazy
Iterable<List<Partition>>
+ * Similar to {@link org.apache.hadoop.hive.metastore.PartitionIterable}, but
+ * each iteration step returns up to {@code batchSize} partitions. Because a
metastore
+ * lookup by name can return fewer objects than names requested (missing
partitions),
+ * the iterator may perform multiple fetches in one {@code next()} until the
batch
+ * is full or partition names are exhausted.
+ * Note that remove() is an illegal call on this, and will result in an
+ * IllegalStateException.
+ */
+public class PartitionsIterable implements Iterable<List<Partition>> {
+
+ private final TableStore tableStore;
+ private final int batchSize;
+ private final TableName tableName;
+ private final List<String> partNames;
+
+ @Override
+ public Iterator<List<Partition>> iterator() {
+ if (partNames.isEmpty()) {
+ return Collections.emptyIterator();
+ }
+
+ return new Iterator<>() {
+ private final Iterator<String> partitionNamesIter = partNames.iterator();
+ private List<Partition> leftParts;
+
+ @Override
+ public boolean hasNext() {
+ return hasPendingPartitions() || partitionNamesIter.hasNext();
+ }
+
+ private boolean hasPendingPartitions() {
+ return leftParts != null && !leftParts.isEmpty();
+ }
+
+ @Override
+ public List<Partition> next() {
+ if (!hasNext()) {
+ throw new NoSuchElementException();
+ }
+ List<Partition> result = new ArrayList<>(batchSize);
+ if (hasPendingPartitions()) {
+ appendPartitions(result, leftParts);
+ leftParts = null;
+ }
+ while (result.size() < batchSize && partitionNamesIter.hasNext()) {
+ List<String> nameBatch = nextNameBatch();
+ appendPartitions(result, fetchBatch(nameBatch));
+ }
+ return result;
+ }
+
+ private void appendPartitions(List<Partition> result, List<Partition>
fetched) {
+ if (fetched.isEmpty()) {
+ return;
+ }
+ int space = batchSize - result.size();
+ if (fetched.size() <= space) {
+ result.addAll(fetched);
+ } else {
+ result.addAll(fetched.subList(0, space));
+ leftParts = new ArrayList<>(fetched.subList(space, fetched.size()));
+ }
+ }
+
+ private List<String> nextNameBatch() {
+ List<String> nameBatch = new ArrayList<>(batchSize);
+ for (int i = 0; i < batchSize && partitionNamesIter.hasNext(); i++) {
+ nameBatch.add(partitionNamesIter.next());
+ }
+ return nameBatch;
+ }
+
+ private List<Partition> fetchBatch(List<String> nameBatch) {
+ try {
+ GetPartitionsArgs.GetPartitionsArgsBuilder argsBuilder =
+ new
GetPartitionsArgs.GetPartitionsArgsBuilder().partNames(nameBatch);
+ List<Partition> partitions =
tableStore.getPartitionsByNames(tableName, argsBuilder.build());
+ return partitions == null ? Collections.emptyList() : partitions;
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+ @Override
+ public void remove() {
+ throw new IllegalStateException(
+ "PartitionsIterable is a read-only iterable and remove() is
unsupported");
+ }
+ };
+ }
+
+ private PartitionsIterable(TableStore tableStore,
+ TableName tableName,
+ int batchSize,
+ List<String> partNames) {
+ this.tableStore = Objects.requireNonNull(tableStore);
+ this.tableName = Objects.requireNonNull(tableName);
+ this.partNames = Objects.requireNonNull(partNames);
+ this.batchSize = batchSize < 0 ? 1000 : batchSize;
Review Comment:
With a zero batch size and nonempty partition names, `next()` returns an
empty list without consuming names, so `hasNext()` stays true and the new
alter/truncate loops never terminate. `MetastoreConf.getIntVar()` does not
enforce the configuration's range validator. Reject zero in the constructor
before an iterator can be created.
--
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]
