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 6: (7 comments) Thanks for the review comments. This change is blocked waiting for https://github.com/apache/iceberg/issues/5882 which we need to include the rollback in our iceberg transaction. 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: // 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? Thanks, I had to rework but this does make the change better and will allow 'EXECUTE' functions to be added more easily. 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: > And maybe this could be 'AlterTableExecuteStmt.createExecuteStmt(table, exp Done 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: > nit: shouldn't we name it to 'rollbackSpec'? yes! 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 Done http://gerrit.cloudera.org:8080/#/c/19002/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/19002/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1336 PS5, Line 1336: > This will have the same problem as https://gerrit.cloudera.org/#/c/19036/ This not done. I believe we need https://github.com/apache/iceberg/issues/5882 to do this. 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: "Execute rollback " > Do we need this MessagaFormat.format here? Thanks, yes I have to change LOG = Logger.getLogger to LOG = LoggerFactory.getLogger to get this to work (with {}) 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 DES This is just what Iceberg does. But it makes sense to me as it makes the rollback non-destructive -- 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: 6 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, 30 Sep 2022 18:57:55 +0000 Gerrit-HasComments: Yes
