Copilot commented on code in PR #6865:
URL: https://github.com/apache/hive/pull/6865#discussion_r4254801699


##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/tools/ColumnDeduplicator.java:
##########
@@ -106,57 +119,53 @@ MetaToolObjectStore.DedupColumnsResult run(String 
catalogFilter, String dbFilter
     return result;
   }
 
-  private void deduplicateTable(TableInfo table, 
MetaToolObjectStore.DedupColumnsResult result) {
+  private PostCommitValidation deduplicateTable(TableInfo table, 
MetaToolObjectStore.DedupColumnsResult result)
+      throws MetaException {
     List<PartitionSdInfo> partitionSds = 
loadPartitionStorageDescriptors(table.tableId);
     if (partitionSds.isEmpty()) {
-      return;
+      return null;
     }
 
     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<List<FieldSchema>, Set<Long>> groups = groupByColumnSchema(cdIds);
 
     Map<Long, Long> cdRemap = new HashMap<>();
-    for (List<Long> group : groups.values()) {
+    for (Set<Long> group : groups.values()) {
       if (group.size() <= 1) {
         continue;
       }
-      long canonicalCdId = pickCanonicalCdId(new HashSet<>(group), 
table.tableCdId, partitionSds);
+      long canonicalCdId = pickCanonicalCdId(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;
+      return null;
     }
-
     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));
+        result.addDetail(String.format("table %s.%s.%s: CD %d -> %d",
+            table.catalogName, table.dbName, table.tableName, partSd.cdId, 
newCdId));

Review Comment:
   The `-verbose` option is documented as printing per-partition details, but 
removing the SD identifier leaves no partition identifier here; multiple 
partitions with the same remap now produce indistinguishable lines. Include the 
newly available partition ID in each detail.
   
   This issue also appears in the following locations of the same file:
   - line 353
   - line 423



-- 
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]

Reply via email to