Copilot commented on code in PR #6887:
URL: https://github.com/apache/hive/pull/6887#discussion_r4332921508
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/tools/ColumnDeduplicator.java:
##########
@@ -119,6 +132,39 @@ MetaToolObjectStore.DedupColumnsResult run(String
catalogFilter, String dbFilter
return result;
}
+ public TableInfo getTable(long tableId) {
+ final MTable table;
+ try {
+ table = pm.getObjectById(MTable.class, new MTable.PK(tableId));
+ } catch (JDOObjectNotFoundException e) {
+ // This table might have been dropped since the beginning
+ return null;
+ }
+ Map<String, String> params = table.getParameters() == null ?
+ new HashMap<>() : table.getParameters();
+ if (clearMarker) {
+ if (!params.containsKey(COLUMN_DEDUPLICATOR_MARKER)) {
+ return null;
+ }
+ params.remove(COLUMN_DEDUPLICATOR_MARKER);
+ table.setParameters(params);
+ return null;
+ }
Review Comment:
`-dryRun` currently does not prevent marker deletion when `clearMarker` is
enabled, because the `clearMarker` branch mutates table parameters
unconditionally. This breaks the “no changes in dry run” contract; either
disallow combining `-dryRun` with `-clearMarker` at CLI validation time or
guard this branch to avoid modifying parameters when `isDryRun` is true.
##########
standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/tools/metatool/TestHiveMetaToolCommandLine.java:
##########
@@ -154,6 +154,9 @@ public void testParseDedupColumns() throws ParseException {
cl = new HiveMetaToolCommandLine(new String[] {"-dedupColumns",
"-timeout", "3600"});
assertEquals(Long.valueOf(3600L), cl.getDedupColumnsTimeoutSeconds());
+
+ cl = new HiveMetaToolCommandLine(new String[] {"-dedupColumns", "hive",
"default", "person", "-clearMarker"});
+ assertTrue(cl.isClearMarker());
Review Comment:
There’s coverage for `-clearMarker` when `-dedupColumns` has all 3 optional
args, but not for the common case where no catalog/db/table filters are
provided (e.g., `-dedupColumns -clearMarker`). With Commons CLI optional-arg
options, a following `-clearMarker` token can be accidentally parsed as an
argument value in some configurations; adding a test for the no-filter case
would ensure the new option is robust and parsed as intended.
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/tools/metatool/MetaToolTaskDedupColumns.java:
##########
@@ -69,10 +70,10 @@ void execute() {
daemon.setDaemon(true);
daemon.start();
}
- result = getObjectStore().dedupColumns(catalogFilter, dbFilter,
tableFilter, progress, isDryRun, isVerbose);
- } catch (MetaException ex) {
- // Deadline.startTimer throws MetaException, this shouldn't happen as
the Deadline has been registered.
- System.err.println(ex.getMessage());
+ result = getObjectStore().dedupColumns(catalogFilter, dbFilter,
tableFilter, progress, isDryRun, isVerbose,
+ getCl().isClearMarker());
+ } catch (Throwable ex) {
+ ex.printStackTrace(System.err);
Review Comment:
Catching `Throwable` here will also swallow `Error`s and (more importantly)
will prevent failures from propagating to `HiveMetaTool.main`, which can lead
to an apparent “successful” run (exit code 0) after printing a stack trace.
Consider catching `Exception` (not `Throwable`) and rethrowing after printing
(or wrapping into a runtime exception), so the process terminates with non-zero
status consistently on failures.
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/tools/ColumnDeduplicator.java:
##########
@@ -119,6 +132,39 @@ MetaToolObjectStore.DedupColumnsResult run(String
catalogFilter, String dbFilter
return result;
}
+ public TableInfo getTable(long tableId) {
Review Comment:
Introducing a new `public` method on `ColumnDeduplicator` expands the
production API surface for what appears to be an internal helper (it’s
primarily used by tests in this PR). If external callers aren’t intended,
consider reducing visibility (and relocating tests to the same package), or
clearly marking it as internal/private API (e.g., via an audience annotation
used in the project) to avoid accidental dependencies.
--
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]
