Copilot commented on code in PR #6887:
URL: https://github.com/apache/hive/pull/6887#discussion_r4327527180
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/tools/ColumnDeduplicator.java:
##########
@@ -119,6 +132,32 @@ MetaToolObjectStore.DedupColumnsResult run(String
catalogFilter, String dbFilter
return result;
}
+ private TableInfo getTable(long tableId) {
+ MTable table = pm.getObjectById(MTable.class, new MTable.PK(tableId));
+ if (table == null) {
+ return null;
+ }
+ Map<String, String> params = table.getParameters() == null ?
+ new HashMap<>() : table.getParameters();
+ // This table has been processed early
+ if (params.containsKey(COLUMN_DEDUPLICATOR_MARKER)) {
+ if (clearMarker) {
+ params.remove(COLUMN_DEDUPLICATOR_MARKER);
+ table.setParameters(params);
+ }
+ return null;
+ }
+
+ if (!isDryRun) {
+ params.put(COLUMN_DEDUPLICATOR_MARKER, "" + System.currentTimeMillis() /
1000);
+ table.setParameters(params);
+ }
Review Comment:
`-clearMarker` mode should not add the marker to tables that don’t already
have it. With the current logic, if `clearMarker` is true and the table is
unmarked, the marker will be added (unless `isDryRun`), which is the opposite
of a “clear-only” operation. Consider short-circuiting early (e.g., return
null) when `clearMarker` is true and the marker is absent, so the cleanup stage
doesn’t mutate previously-unmarked tables.
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/tools/ColumnDeduplicator.java:
##########
@@ -229,25 +268,16 @@ private List<TableInfo> findPartitionedTables(String
catalogFilter, String dbFil
Query query = !filter.isEmpty() ?
pm.newQuery(MPartition.class, filter.toString()) :
pm.newQuery(MPartition.class);
- query.setResult("DISTINCT this.table");
+ query.setResult("DISTINCT this.table.id");
boolean success = false;
- List<TableInfo> tables = new ArrayList<>();
+ List<Long> tables = new ArrayList<>();
store.openTransaction();
try {
- List<MTable> mTables = (List<MTable>)
query.executeWithArray(parameterVals.toArray(new String[0]));
- pm.retrieveAll(mTables);
- for (MTable mTable : mTables) {
- pm.retrieve(mTable.getDatabase());
- pm.retrieve(mTable.getSd());
- pm.retrieve(mTable.getSd().getCD());
- tables.add(new TableInfo(
- mTable.getId(),
- mTable.getSd().getCD().getId(),
- mTable.getDatabase().getCatalogName(),
- mTable.getDatabase().getName(),
- mTable.getTableName()));
- }
+ tables.addAll((List<Long>)
query.executeWithArray(parameterVals.toArray(new String[0])));
success = store.commitTransaction();
+ } catch (Throwable e) {
+ e.printStackTrace(System.err);
+ throw new RuntimeException(e);
Review Comment:
Throwing a `RuntimeException` here can terminate the metatool run
unexpectedly (and bypass the `DedupColumnsResult` failure-reporting path that
the rest of `run()` uses). To preserve the PR’s stated goal of being more
error-tolerant, prefer propagating a checked exception type already handled by
the caller (e.g., `MetaException`), or handle it inside `run()` by setting
`result.catchException(e)` and returning a result with the captured throwable
instead of crashing the process.
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/tools/metatool/HiveMetaTool.java:
##########
@@ -81,6 +81,7 @@ public static void main(String[] args) {
execute(args);
} catch (Exception e) {
status = -1;
+ e.printStackTrace(System.err);
LOGGER.error("Exception occured", e);
Review Comment:
This prints the full stack trace twice (once to `stderr`, once via the
logger). If the intention is to ensure visibility in console output, consider
printing a concise message to `stderr` (or guarding stacktrace printing behind
a verbosity flag) while keeping the full stack trace in logs, to reduce
noisy/duplicated output in operational environments.
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/tools/metatool/MetaToolTaskDedupColumns.java:
##########
@@ -69,7 +69,8 @@ void execute() {
daemon.setDaemon(true);
daemon.start();
}
- result = getObjectStore().dedupColumns(catalogFilter, dbFilter,
tableFilter, progress, isDryRun, isVerbose);
+ result = getObjectStore().dedupColumns(catalogFilter, dbFilter,
tableFilter, progress, isDryRun, isVerbose,
+ getCl().isClearMarker());
Review Comment:
Given `ColumnDeduplicator.findPartitionedTables()` now throws
`RuntimeException` on failures, this call path can escape the existing `catch
(MetaException ex)` below and abort the task without the intended structured
error handling/summary. Either avoid throwing `RuntimeException` from the dedup
implementation (preferred), or broaden the catch here to handle unchecked
exceptions consistently so the command doesn’t fail in an uncontrolled way.
--
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]
