Andrew Sherman 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 9: (9 comments) Comments ahead of next patch (coming soon) http://gerrit.cloudera.org:8080/#/c/19002/9/common/thrift/JniCatalog.thrift File common/thrift/JniCatalog.thrift: http://gerrit.cloudera.org:8080/#/c/19002/9/common/thrift/JniCatalog.thrift@431 PS9, Line 431: 1: required TRollbackType kind > nit: this could work without the 'kind' member by simply checking which one Thanks, you're right, but this seems robust and extensible. http://gerrit.cloudera.org:8080/#/c/19002/9/common/thrift/JniCatalog.thrift@434 PS9, Line 434: 2: optional i64 timestamp_millis > Kind of unrelated and I might be totally off here, but the other day I saw Yes, if I understand it right, the 'timestamp" here is a really milliseconds, that Impala timestamp is nanoseconds and so needs more space. http://gerrit.cloudera.org:8080/#/c/19002/9/fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteExpireSnapshotsStmt.java File fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteExpireSnapshotsStmt.java: http://gerrit.cloudera.org:8080/#/c/19002/9/fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteExpireSnapshotsStmt.java@36 PS9, Line 36: This uses the ExpireSnapshot API to : * expire snapshots, it calls the ExpireSnapshot.expireOlderThan(timestampMillis) method. : * TableProperties.MIN_SNAPSHOTS_TO_KEEP table property manages how many snapshots : * should be retained even when all snapshots are selected by expireOlderThan(). > this comment describes the code that is rather in IcebergCatalogOpExecutor Done in next patch http://gerrit.cloudera.org:8080/#/c/19002/9/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/9/fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteRollbackStmt.java@58 PS9, Line 58: super.analyze(analyzer); : FeTable table = getTargetTable(); : if (!(table instanceof FeIcebergTable)) { : throw new AnalysisException("ALTER TABLE EXECUTE ROLLBACK is only supported " : + "for Iceberg tables: " + table.getTableName()); : } : analyzeFunctionCallExpr(analyzer, USAGE); : analyzeParameter(analyzer); > AlterTableExecuteRollbackStmt.analyze() and AlterTableExecuteExpireSnapshot Nice idea, that *is* the purpose of AlterTableExecuteStmt. I tried this, but the differing USAGE strings made it untidy. So maybe keep it like this, we'll know better when there is a third EXECUTE statement. http://gerrit.cloudera.org:8080/#/c/19002/9/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/9/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java@199 PS9, Line 199: long snapshotId = params.getSnapshot_id(); : long timestampMillis = params.getTimestamp_millis(); > These are optional members of TAlterTableExecuteRollbackParams. The values I added some Preconditions.check() calls in the next patch http://gerrit.cloudera.org:8080/#/c/19002/9/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java@215 PS9, Line 215: manageSnapshots.rollbackTo(snapshotId); > just to be on the safe side let me ask: apart from IllegalStateException th This is tested in test_execute_rollback, the output we get is: Query aborted:ValidationException: Cannot roll back to snapshot, not an ancestor of the current state: 3922115053442881239 Is that sufficient do you think? http://gerrit.cloudera.org:8080/#/c/19002/9/tests/query_test/test_iceberg.py File tests/query_test/test_iceberg.py: http://gerrit.cloudera.org:8080/#/c/19002/9/tests/query_test/test_iceberg.py@245 PS9, Line 245: def test_execute_rollback(self, vector, unique_database): > Another use case that might be worth testing is when we set the timezone to There is an example in iceberg_rollback.test http://gerrit.cloudera.org:8080/#/c/19002/9/tests/query_test/test_iceberg.py@247 PS9, Line 247: self.run_test_case('QueryTest/iceberg-rollback', vector, use_db=unique_database) > I'd execute these in a separate test, e.g. test_execute_rollback_negative Done in next patch set. http://gerrit.cloudera.org:8080/#/c/19002/9/tests/query_test/test_iceberg.py@271 PS9, Line 271: # We cannot roll back to a snapshot that is not a current ancestor. > This just made me think if there is a way to un-do a rollback if we realise I think you cannot do this. This is case where having a branch would help, if you remember to make the branch before doing the rollback. -- 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: 9 Gerrit-Owner: Andrew Sherman <asher...@cloudera.com> Gerrit-Reviewer: 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: Fri, 03 Mar 2023 23:56:15 +0000 Gerrit-HasComments: Yes
