Copilot commented on code in PR #6592:
URL: https://github.com/apache/hive/pull/6592#discussion_r3558167734
##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/properties/HMSPropertyManager.java:
##########
@@ -45,14 +51,13 @@ public class HMSPropertyManager extends PropertyManager {
/** Table maintenance operation type. */
public enum MaintenanceOpType {
- COMPACTION,
- SNAPSHOT_EXPIRY,
- STATS_REBUILD,
- MV_BUILD,
- MV_REFRESH,
- SHUFFLE_TO_NEW_PART,
- RECOMPRESS,
- REORG
+ EXPIRE_SNAPSHOT,
+ REWRITE_DATA_FILES,
+ COMPRESS_DATA_FILES,
+ REWRITE_MANIFEST,
Review Comment:
Renaming/removing enum constants here is a wire/storage compatibility break:
these values are serialized as strings via
PropertyType.write(DataOutput)->writeUTF(format(value)) and deserialized via
PropertyType.read(DataInput)->parse(readUTF()). Since parse() uses
MaintenanceOpType.valueOf(...), any previously persisted value like
COMPACTION/SNAPSHOT_EXPIRY will now throw IllegalArgumentException when reading
existing maps or handling older clients/scripts. Consider keeping legacy
constants (deprecated) or adding explicit legacy-name mapping during parsing to
preserve backward compatibility.
##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/properties/HMSPropertyManager.java:
##########
@@ -73,12 +78,7 @@ public static MaintenanceOpType findOpType(int ordinal) {
/** Table maintenance operation status. */
public enum MaintenanceOpStatus {
- MAINTENANCE_NEEDED,
- SCHEDULED,
- IN_PROGRESS,
- DONE,
- CLEANUP_NEEDED,
- FAILED
+ INIT, CANCELED, SUBMITTED, COMPLETED, FAILED
}
Review Comment:
Renaming/removing enum constants here is a wire/storage compatibility break
for the same reason as MaintenanceOpType: PropertyType.read() calls
parse(readUTF()), and parse() uses MaintenanceOpStatus.valueOf(...). Any
previously persisted/posted values (e.g.,
MAINTENANCE_NEEDED/SCHEDULED/IN_PROGRESS/...) will now fail to parse and can
cause runtime exceptions when reading existing property maps or serving older
clients.
##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/properties/PropertyStore.java:
##########
@@ -84,7 +84,7 @@ public abstract class PropertyStore {
/**
* Persists an iterator property map.
- * <p>May be useful to override to use one transaction.</p>
+ * <p>Maybe useful to override to use one transaction.</p>
Review Comment:
The updated Javadoc sentence reads less correctly: “Maybe useful …” should
be “May be useful …”.
--
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]
