ayushtkn commented on code in PR #4797:
URL: https://github.com/apache/hive/pull/4797#discussion_r1356928414
##########
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/IcebergTableUtil.java:
##########
@@ -237,11 +238,18 @@ public static void rollback(Table table,
AlterTableExecuteSpec.RollbackSpec.Roll
* @param table the iceberg table
* @param value parameter of the rollback, that can be a timestamp in millis
or a snapshot id
*/
- public static void setCurrentSnapshot(Table table, Long value) {
+ public static void setCurrentSnapshot(Table table, String value) {
ManageSnapshots manageSnapshots = table.manageSnapshots();
+ long snapshotId;
+ try {
+ snapshotId = Long.valueOf(value);
+ } catch (NumberFormatException e) {
+ snapshotId = Optional.ofNullable(table.refs().get(value)).map(ref ->
ref.snapshotId()).orElseThrow(() ->
Review Comment:
Can change to
```
snapshotId =
Optional.ofNullable(table.refs().get(value)).map(SnapshotRef::snapshotId).orElseThrow(()
->
```
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/AlterTableExecuteSpec.java:
##########
@@ -165,19 +165,19 @@ public String toString() {
* </ul>
*/
public static class SetCurrentSnapshotSpec {
- private final long snapshotId;
+ private final String snapshotIdOrRefName;
Review Comment:
Change the javadoc above
```
* <li>snapshot Id: it should be a valid snapshot version</li>
```
##########
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/IcebergTableUtil.java:
##########
@@ -237,11 +238,18 @@ public static void rollback(Table table,
AlterTableExecuteSpec.RollbackSpec.Roll
* @param table the iceberg table
* @param value parameter of the rollback, that can be a timestamp in millis
or a snapshot id
*/
- public static void setCurrentSnapshot(Table table, Long value) {
+ public static void setCurrentSnapshot(Table table, String value) {
ManageSnapshots manageSnapshots = table.manageSnapshots();
+ long snapshotId;
+ try {
+ snapshotId = Long.valueOf(value);
Review Comment:
redundant boxing here, snapshotId is primitive, should use
```
snapshotId = Long.parseLong(value);
```
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/AlterTableExecuteSpec.java:
##########
@@ -165,19 +165,19 @@ public String toString() {
* </ul>
*/
public static class SetCurrentSnapshotSpec {
- private final long snapshotId;
+ private final String snapshotIdOrRefName;
- public SetCurrentSnapshotSpec(Long snapshotId) {
- this.snapshotId = snapshotId;
+ public SetCurrentSnapshotSpec(String snapshotIdOrRefName) {
+ this.snapshotIdOrRefName = snapshotIdOrRefName;
}
- public Long getSnapshotId() {
- return snapshotId;
+ public String getSnapshotIdOrRefName() {
+ return snapshotIdOrRefName;
}
@Override
public String toString() {
- return MoreObjects.toStringHelper(this).add("snapshotId",
snapshotId).toString();
+ return MoreObjects.toStringHelper(this).add("snapshotId",
snapshotIdOrRefName).toString();
Review Comment:
it can be either id or ref, can we change ``snapshotId`` to something common?
##########
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/IcebergTableUtil.java:
##########
@@ -237,11 +238,18 @@ public static void rollback(Table table,
AlterTableExecuteSpec.RollbackSpec.Roll
* @param table the iceberg table
* @param value parameter of the rollback, that can be a timestamp in millis
or a snapshot id
*/
- public static void setCurrentSnapshot(Table table, Long value) {
+ public static void setCurrentSnapshot(Table table, String value) {
ManageSnapshots manageSnapshots = table.manageSnapshots();
+ long snapshotId;
+ try {
+ snapshotId = Long.valueOf(value);
+ } catch (NumberFormatException e) {
+ snapshotId = Optional.ofNullable(table.refs().get(value)).map(ref ->
ref.snapshotId()).orElseThrow(() ->
+ new IllegalArgumentException(String.format("SnapshotRef %s does not
exist", value)));
+ }
LOG.debug("Rolling the iceberg table {} from snapshot id {} to snapshot ID
{}", table.name(),
Review Comment:
can you update the log and javadoc accordingly that we now support ID+ref
--
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]
