Copilot commented on code in PR #6690:
URL: https://github.com/apache/hive/pull/6690#discussion_r3780315584
##########
ql/src/java/org/apache/hadoop/hive/ql/metadata/HiveStorageHandler.java:
##########
@@ -901,6 +901,15 @@ default boolean
canUseTruncate(org.apache.hadoop.hive.ql.metadata.Table hmsTable
return true;
}
+ /**
+ * Validates that partition compaction can resolve a unique partition spec
for the given partition name.
+ * @param hmsTable table metadata stored in Hive Metastore
+ * @param partitionName fully qualified partition name
+ */
+ default void
validateCompactionPartition(org.apache.hadoop.hive.ql.metadata.Table hmsTable,
String partitionName)
+ throws HiveException {
+ }
Review Comment:
The Javadoc says `partitionName` is a "fully qualified partition name", but
the Iceberg implementation and `IcebergTableUtil.getPartitionSpec` expect a
Hive-style partition *path* like `dept_id=2` (or `a=1/b=2`). Updating the
Javadoc avoids confusion for other storage handler implementers/callers.
##########
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/IcebergTableUtil.java:
##########
@@ -775,15 +775,25 @@ public static PartitionSpec getPartitionSpec(Table
icebergTable, String partitio
// Extract field names from the path: "field1=val1/field2=val2" → [field1,
field2]
List<String> fieldNames =
Lists.newArrayList(Warehouse.makeSpecFromName(partitionPath).keySet());
- return icebergTable.specs().values().stream()
+ List<PartitionSpec> matches = icebergTable.specs().values().stream()
.filter(spec -> {
List<String> specFieldNames = spec.fields().stream()
.map(PartitionField::name)
.toList();
return specFieldNames.equals(fieldNames);
})
- .findFirst() // Supposed to be only one matching spec
- .orElseThrow(() -> new HiveException("No matching partition spec found
for partition path: " + partitionPath));
+ .toList();
+
+ if (matches.size() > 1) {
+ throw new HiveException(String.format(
+ "Ambiguous partition spec for partition path %s: matched spec ids
%s",
+ partitionPath,
+
matches.stream().map(PartitionSpec::specId).map(String::valueOf).collect(Collectors.joining(",
"))));
+ }
Review Comment:
The ambiguous-spec error message builds the spec id list from
`icebergTable.specs().values()` iteration order, which can be non-deterministic
depending on the backing map implementation. Sorting the spec ids makes the
message stable (and prevents test flakiness / log churn).
##########
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/HiveIcebergStorageHandler.java:
##########
@@ -2209,6 +2209,20 @@ Boolean hasAppendsOnly(Iterable<Snapshot> snapshots,
SnapshotContext since) {
return null;
}
+ @Override
+ public void
validateCompactionPartition(org.apache.hadoop.hive.ql.metadata.Table hmsTable,
String partitionName)
+ throws HiveException {
+ Table table = IcebergTableUtil.getTable(conf, hmsTable.getTTable());
+ if (!IcebergTableUtil.hasUndergonePartitionEvolution(table)) {
+ return;
+ }
+ try {
+ IcebergTableUtil.getPartitionSpec(table, partitionName);
+ } catch (MetaException e) {
+ throw new HiveException(e);
+ }
+ }
Review Comment:
When the partition spec resolution fails, the exception currently bubbles up
as a generic `HiveException` message from `IcebergTableUtil.getPartitionSpec`.
Wrapping it here with compaction context (table + operation) would make
failures easier to diagnose for users running `ALTER TABLE .. COMPACT
PARTITION(...)`.
--
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]
