Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/19002 )
Change subject: IMPALA-11482: Alter Table Execute Rollback for Iceberg tables. ...................................................................... Patch Set 5: (6 comments) Looks great! Left a couple of comments. http://gerrit.cloudera.org:8080/#/c/19002/5/common/thrift/JniCatalog.thrift File common/thrift/JniCatalog.thrift: http://gerrit.cloudera.org:8080/#/c/19002/5/common/thrift/JniCatalog.thrift@428 PS5, Line 428: // Parameters for ALTER TABLE EXECUTE ROLLBACK operations. : struct TAlterTableExecuteRollbackParams { : // Is rollback to a date or snapshot id. : 1: required TRollbackType kind : : // If kind is TIME_ID this is the date to rollback to. : 2: optional i64 timestamp_millis : : // If kind is VERSION_ID this is the id to rollback to. : 3: optional i64 snapshot_id : } Just an idea: can we embed this under TAlterTableExecuteParams? E.g. TAlterTableExecuteParams could look like: struct TAlterTableExecuteParams { 1: optional TExecuteExpireSnapshotsParams 2: optional TExecuteRollbackParams } This way we wouldn't need to add a new field to TAlterTableParams whenever we invent a new EXECUTE command. http://gerrit.cloudera.org:8080/#/c/19002/5/fe/src/main/cup/sql-parser.cup File fe/src/main/cup/sql-parser.cup: http://gerrit.cloudera.org:8080/#/c/19002/5/fe/src/main/cup/sql-parser.cup@1323 PS5, Line 1323: new AlterTableExecuteStmt(table, expr); And maybe this could be 'AlterTableExecuteStmt.createExecuteStmt(table, expr);' And it could return either AlterTableExecuteRollbackStmt or AlterTableExecuteExpireSnapshotsStmt based on the name of the function. This way we also don't need KW_ROLLBACK. http://gerrit.cloudera.org:8080/#/c/19002/5/fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteRollbackStmt.java File fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteRollbackStmt.java: http://gerrit.cloudera.org:8080/#/c/19002/5/fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteRollbackStmt.java@34 PS5, Line 34: timeTravelSpec nit: shouldn't we name it to 'rollbackSpec'? http://gerrit.cloudera.org:8080/#/c/19002/5/fe/src/main/java/org/apache/impala/analysis/TableRef.java File fe/src/main/java/org/apache/impala/analysis/TableRef.java: http://gerrit.cloudera.org:8080/#/c/19002/5/fe/src/main/java/org/apache/impala/analysis/TableRef.java@478 PS5, Line 478: nit: indentation is off http://gerrit.cloudera.org:8080/#/c/19002/5/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/19002/5/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java@199 PS5, Line 199: MessageFormat.format( Do we need this MessagaFormat.format here? log4j.logger already provides these methods with formatting capabilities, i.e. it could be just: LOG.info("Execute rollback " + "kind={0} snapshotId={1} timestamp={2}", kind, snapshotId, timestampMillis); http://gerrit.cloudera.org:8080/#/c/19002/5/tests/query_test/test_iceberg.py File tests/query_test/test_iceberg.py: http://gerrit.cloudera.org:8080/#/c/19002/5/tests/query_test/test_iceberg.py@258 PS5, Line 258: # We rolled back, but that creates a new snapshot, so now there are 4. : snapshots = self.get_snapshots(self.client, tbl_name, expected_result_size=4) It's interesting that non-ancestor snapshots are still in the output of DESCRIBE HISTORY, but we cannot rollback to them, not even by snapshot id. I checked Hive and it works the same so it should be OK. -- To view, visit http://gerrit.cloudera.org:8080/19002 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic74913d3b81103949ffb5eef7cc936303494f8b9 Gerrit-Change-Number: 19002 Gerrit-PatchSet: 5 Gerrit-Owner: Andrew Sherman <asher...@cloudera.com> Gerrit-Reviewer: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Gergely Fürnstáhl <gfurnst...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Tamas Mate <tma...@apache.org> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Wed, 21 Sep 2022 11:50:40 +0000 Gerrit-HasComments: Yes
