Copilot commented on code in PR #6719:
URL: https://github.com/apache/hive/pull/6719#discussion_r3840570242
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/tools/metatool/HiveMetaToolCommandLine.java:
##########
@@ -174,16 +192,19 @@ private void parseCommandLine(String[] args) throws
ParseException {
listExtTblLocsParams = cl.getOptionValues(LIST_EXT_TBL_LOCS.getOpt());
diffExtTblLocsParams = cl.getOptionValues(DIFF_EXT_TBL_LOCS.getOpt());
dryRun = cl.hasOption(DRY_RUN.getOpt());
+ verbose = cl.hasOption(VERBOSE.getOpt());
serdePropKey = cl.getOptionValue(SERDE_PROP_KEY.getOpt());
tablePropKey = cl.getOptionValue(TABLE_PROP_KEY.getOpt());
help = cl.hasOption(HELP.getOpt());
metadataSummaryParams = cl.getOptionValues(METADATA_SUMMARY.getOpt());
+ dedupColumnsParams = cl.getOptionValues(DEDUP_COLUMNS.getOpt());
Review Comment:
`isDedupColumns()` is based on `dedupColumnsParams != null`, but
`getOptionValues()` can return null when the option is present with *no* args.
Since `-dedupColumns` is documented as having optional filters, `metatool
-dedupColumns` (no filters) can be treated as "no task" and fail parsing.
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/tools/metatool/HiveMetaToolCommandLine.java:
##########
@@ -201,9 +222,18 @@ private void parseCommandLine(String[] args) throws
ParseException {
diffExtTblLocsParams.length + " arguments");
}
- if ((dryRun || serdePropKey != null || tablePropKey != null) &&
!isUpdateLocation()) {
- throw new IllegalArgumentException("-dryRun, -serdePropKey,
-tablePropKey may be used only for the " +
- "-updateLocation command");
+ if ((dryRun || serdePropKey != null || tablePropKey != null) &&
!isUpdateLocation()
+ && !isDedupColumns()) {
+ throw new IllegalArgumentException("-dryRun, -serdePropKey,
-tablePropKey may be used only for the "
+ + "-updateLocation or -dedupColumns commands");
+ }
Review Comment:
The "not allowed" error for `-dryRun/-serdePropKey/-tablePropKey` currently
claims `-serdePropKey`/`-tablePropKey` are valid for `-dedupColumns`, but a
later check correctly forbids them unless `-updateLocation`. This makes the
thrown message misleading for cases like `-listFSRoot -serdePropKey abc`.
Consider separating the validations so the error message matches what is
actually supported.
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/tools/ColumnDeduplicator.java:
##########
@@ -0,0 +1,372 @@
+/*
+ * 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.tools;
+
+import org.apache.commons.collections4.CollectionUtils;
+import org.apache.hadoop.hive.metastore.RawStore;
+import org.apache.hadoop.hive.metastore.api.FieldSchema;
+import org.apache.hadoop.hive.metastore.api.MetaException;
+import org.apache.hadoop.hive.metastore.metastore.RawStoreBundle;
+import org.apache.hadoop.hive.metastore.model.MColumnDescriptor;
+import org.apache.hadoop.hive.metastore.model.MConstraint;
+import org.apache.hadoop.hive.metastore.model.MPartition;
+import org.apache.hadoop.hive.metastore.model.MStorageDescriptor;
+import org.apache.hadoop.hive.metastore.model.MTable;
+
+import javax.jdo.JDOHelper;
+import javax.jdo.PersistenceManager;
+import javax.jdo.Query;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.atomic.AtomicReference;
+import java.util.stream.Collectors;
+
+import static
org.apache.hadoop.hive.metastore.ObjectStore.appendPatternCondition;
+import static
org.apache.hadoop.hive.metastore.metastore.impl.TableStoreImpl.convertToFieldSchemas;
+import static
org.apache.hadoop.hive.metastore.metastore.impl.TableStoreImpl.hasRemainingCDReference;
+import static org.apache.hadoop.hive.metastore.utils.StringUtils.isEmpty;
+
+/**
+ * De-duplicates column descriptors (CDs) for partitioned tables in the
metastore.
+ * Identical column schemas within a table are merged so that partitions share
+ * the same CD, reducing metadata bloat that can accumulate during replication.
+ */
+final class ColumnDeduplicator {
+ private final RawStore store;
+ private final PersistenceManager pm;
+ private final AtomicReference<String> progress;
+ private final boolean isDryRun;
+ private final boolean isVerbose;
+
+ ColumnDeduplicator(RawStoreBundle bundle, AtomicReference<String> progress,
+ boolean isDryRun, boolean isVerbose) {
+ this.store = bundle.getBaseStore();
+ this.pm = bundle.getPersistentManager();
+ this.progress = progress;
+ this.isDryRun = isDryRun;
+ this.isVerbose = isVerbose;
+ }
+
+ MetaToolObjectStore.DedupColumnsResult run(String catalogFilter, String
dbFilter, String tableFilter) {
+ List<TableInfo> tables = findPartitionedTables(catalogFilter, dbFilter,
tableFilter);
+ MetaToolObjectStore.DedupColumnsResult result = new
MetaToolObjectStore.DedupColumnsResult(tables.size());
+
+ long start = System.currentTimeMillis();
+ for (int i = 0; i < tables.size() && result.getException() == null; i++) {
+ boolean committed = false;
+ TableInfo table = tables.get(i);
+ store.openTransaction();
+ try {
+ deduplicateTable(table, result);
+ committed = store.commitTransaction();
+ } catch (Exception ex) {
+ result.catchException(ex);
+ } finally {
+ if (!committed) {
+ store.rollbackTransaction();
+ if (result.getException() == null) {
+ result.catchException(
+ new MetaException("Failed to apply column descriptor
de-duplication updates for table " + table));
+ }
+ }
+ }
+ if (progress != null) {
+ progress.set(String.format(
+ "Finished %d tables in %d total tables, time taken: %d ms, columns
updated: %d, removed: %d",
+ (i + 1),
+ result.getTablesScanned(),
+ (System.currentTimeMillis() - start),
+ result.getStorageDescriptorsUpdated(),
+ result.getColumnDescriptorsRemoved()));
+ }
+ }
+ return result;
+ }
+
+ private void deduplicateTable(TableInfo table,
MetaToolObjectStore.DedupColumnsResult result) throws MetaException {
+ List<PartitionSdInfo> partitionSds =
loadPartitionStorageDescriptors(table.tableId);
+ if (partitionSds.isEmpty()) {
+ return;
+ }
+
+ Set<Long> cdIds = partitionSds.stream().map(p ->
p.cdId).collect(Collectors.toSet());
+ cdIds.add(table.tableCdId);
+
+ Map<Long, List<FieldSchema>> cdColumns = loadColumnSchemas(cdIds);
+ Map<List<FieldSchema>, List<Long>> groups = groupByColumnSchema(cdColumns);
+
+ Map<Long, Long> cdRemap = new HashMap<>();
+ for (List<Long> group : groups.values()) {
+ if (group.size() <= 1) {
+ continue;
+ }
+ long canonicalCdId = pickCanonicalCdId(new HashSet<>(group),
table.tableCdId, partitionSds);
+ for (long cdId : group) {
+ if (cdId != canonicalCdId) {
+ cdRemap.put(cdId, canonicalCdId);
+ }
+ }
+ }
+
+ if (cdRemap.isEmpty()) {
+ return;
+ }
+
+ List<Map.Entry<PartitionSdInfo, Long>> partSdUpdates =
buildPartitionUpdates(partitionSds, cdRemap);
+ if (partSdUpdates.isEmpty()) {
+ return;
+ }
+
+ result.incrementTablesWithDuplicates();
+ for (Map.Entry<PartitionSdInfo, Long> update : partSdUpdates) {
+ result.incrementStorageDescriptorsUpdated();
+ if (isVerbose) {
+ PartitionSdInfo partSd = update.getKey();
+ long newCdId = update.getValue();
+ result.addDetail(String.format("table %s.%s.%s: SD %s CD %d -> %d",
+ table.catalogName, table.dbName, table.tableName,
+ JDOHelper.getObjectId(partSd.sd), partSd.cdId, newCdId));
+ }
+ }
+ if (!isDryRun) {
+ applyTableChanges(partSdUpdates, result);
+ } else {
+ Set<Long> candidateCdIds = new HashSet<>();
+ for (Map.Entry<PartitionSdInfo, Long> update : partSdUpdates) {
+ candidateCdIds.add(update.getKey().cdId);
+ }
+
result.addColumnDescriptorsRemoved(countRemovableColumnDescriptors(candidateCdIds));
+ }
Review Comment:
In dry-run mode, `countRemovableColumnDescriptors()` checks current DB
references via `hasRemainingCDReference(...)` *before* any remapping is
applied, so it will typically report 0 removable CDs even when the non-dry-run
path would remove some after updating SD->CD references. Consider either
omitting this metric in dry-run output or computing removals based on the
post-update reference state (e.g., excluding the SDs that would be updated).
--
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]
