Copilot commented on code in PR #6865:
URL: https://github.com/apache/hive/pull/6865#discussion_r4253226743
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/tools/ColumnDeduplicator.java:
##########
@@ -278,7 +299,87 @@ private Map<Long, List<FieldSchema>>
loadColumnSchemas(Set<Long> cdIds) {
return result;
}
- private static Map<List<FieldSchema>, List<Long>>
groupByColumnSchema(Map<Long, List<FieldSchema>> cdColumns) {
+ private List<PartitionSchemaSnapshot>
capturePartitionSchemas(List<Map.Entry<PartitionSdInfo, Long>> updates) {
+ List<PartitionSchemaSnapshot> snapshots = new ArrayList<>(updates.size());
+ for (Map.Entry<PartitionSdInfo, Long> update: updates) {
+ PartitionSdInfo psi = update.getKey();
+ snapshots.add(new PartitionSchemaSnapshot(
+ psi.partId,
+ convertToFieldSchemas(psi.sd.getCD().getCols())));
Review Comment:
The validation snapshot records only the pre-update schema, not the
canonical CD ID from `update.getValue()`. If an SD remap is not persisted, the
partition still exposes the same schema through its old duplicate CD, so this
check passes and `tablesValidated` is incremented even though de-duplication
did not occur. Record the expected canonical CD ID in each snapshot and compare
it with the reloaded `partition.cdId` in addition to comparing schemas.
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/tools/ColumnDeduplicator.java:
##########
@@ -93,6 +96,16 @@ MetaToolObjectStore.DedupColumnsResult run(String
catalogFilter, String dbFilter
}
}
}
+
+ try {
+ if (committed && validation != null) {
+ validateTableAfterDedup(table, validation.schemaSnapshots());
+ result.incrementTablesValidated();
+ }
+ } catch (MetaException e) {
+ result.catchException(new IllegalStateException("Failed to validate
the column deduplication for this table: " + table, e));
Review Comment:
The validation query can throw unchecked JDO/database exceptions, but this
catches only `MetaException`. Such failures escape `run()`, bypass
`DedupColumnsResult`, and prevent `MetaToolTaskDedupColumns` from printing the
result summary and its normal wrapped failure. Catch `Exception` here, as the
mutation phase already does, so every validation failure follows the same
result contract.
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/tools/metatool/MetaToolTaskDedupColumns.java:
##########
@@ -110,6 +110,9 @@ private void
printSummary(MetaToolObjectStore.DedupColumnsResult result, boolean
+ result.getStorageDescriptorsUpdated());
if (!isDryRun) {
System.out.println("Column descriptors removed: " +
result.getColumnDescriptorsRemoved());
+ if (result.getTablesValidated() > 0) {
+ System.out.println("Post-commit validation passed for " +
result.getTablesValidated() + " table(s).");
+ }
Review Comment:
This adds observable CLI output for live runs, while the PR description says
the change has no user-facing impact. Please update the PR's user-facing-change
section to mention the new validation summary (and the updated command help) so
release reviewers and users are not given conflicting information.
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/tools/ColumnDeduplicator.java:
##########
@@ -278,7 +299,87 @@ private Map<Long, List<FieldSchema>>
loadColumnSchemas(Set<Long> cdIds) {
return result;
}
- private static Map<List<FieldSchema>, List<Long>>
groupByColumnSchema(Map<Long, List<FieldSchema>> cdColumns) {
+ private List<PartitionSchemaSnapshot>
capturePartitionSchemas(List<Map.Entry<PartitionSdInfo, Long>> updates) {
+ List<PartitionSchemaSnapshot> snapshots = new ArrayList<>(updates.size());
+ for (Map.Entry<PartitionSdInfo, Long> update: updates) {
+ PartitionSdInfo psi = update.getKey();
+ snapshots.add(new PartitionSchemaSnapshot(
+ psi.partId,
+ convertToFieldSchemas(psi.sd.getCD().getCols())));
+ }
+ return snapshots;
+ }
+
+ public void validateTableAfterDedup(TableInfo table,
List<PartitionSchemaSnapshot> before)
+ throws MetaException {
+ List<PartitionSdInfo> partitions =
+ loadPartitionCdRefs(before.stream().map(b -> b.partitionId).toList());
Review Comment:
This sends every updated partition ID in one JDO collection query. Large
partitioned tables can exceed backend/generated-query limits after the
de-duplication transaction has already committed, causing the command to report
a validation failure for a successful mutation. Please batch this reload with
`Batchable.runBatched` using the raw-store JDO batch setting;
`MetastoreConf.RAWSTORE_PARTITION_BATCH_SIZE` explicitly exists for generated
JDO queries that become too large (MetastoreConf.java:1826-1830), and analogous
ID-list reads are batched in `DirectSqlUpdatePart.java:670-687`.
--
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]
