zabetak commented on code in PR #4859:
URL: https://github.com/apache/hive/pull/4859#discussion_r1410480468
##########
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/storage/compact/AlterTableCompactOperation.java:
##########
@@ -80,15 +84,15 @@ public AlterTableCompactOperation(DDLOperationContext
context, AlterTableCompact
Optional<String> partitionName =
partitionMap.keySet().stream().findFirst();
partitionName.ifPresent(compactionRequest::setPartitionname);
}
- List<CompactionResponse> compactionResponses =
+ Map<CompactionResponse, String> compactionResponses =
initiatorBase.initiateCompactionForTable(compactionRequest,
table.getTTable(), partitionMap);
- for (CompactionResponse compactionResponse : compactionResponses) {
+ for (Map.Entry<CompactionResponse, String> compactionResponseEntry :
compactionResponses.entrySet()) {
+ CompactionResponse compactionResponse = compactionResponseEntry.getKey();
if (!compactionResponse.isAccepted()) {
- String message;
if (compactionResponse.isSetErrormessage()) {
- message = compactionResponse.getErrormessage();
throw new HiveException(ErrorMsg.COMPACTION_REFUSED,
table.getDbName(), table.getTableName(),
- "CompactionId: " + compactionResponse.getId(), message);
+ compactionResponseEntry.getValue() == null ? "" :
+ "(partition=" + compactionResponseEntry.getValue() + ")",
compactionResponse.getErrormessage());
Review Comment:
Do we have a test case that verifies that partition is present in the
Exception? If not can you please add one.
##########
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/storage/compact/AlterTableCompactOperation.java:
##########
@@ -80,15 +84,15 @@ public AlterTableCompactOperation(DDLOperationContext
context, AlterTableCompact
Optional<String> partitionName =
partitionMap.keySet().stream().findFirst();
partitionName.ifPresent(compactionRequest::setPartitionname);
}
- List<CompactionResponse> compactionResponses =
+ Map<CompactionResponse, String> compactionResponses =
initiatorBase.initiateCompactionForTable(compactionRequest,
table.getTTable(), partitionMap);
- for (CompactionResponse compactionResponse : compactionResponses) {
+ for (Map.Entry<CompactionResponse, String> compactionResponseEntry :
compactionResponses.entrySet()) {
Review Comment:
Instead of creating and passing a `Map` around why don't we extract the
partition iteration here instead of having it inside the
`initiateCompactionForTable`? This way we will not need a Map and will avoid
the need for iterating over the partitions twice (once inside the
`initiateCompactionForTable` and once over the respective responses here).
--
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: gitbox-unsubscr...@hive.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscr...@hive.apache.org
For additional commands, e-mail: gitbox-h...@hive.apache.org
