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 8: (7 comments) Thank for the useful comments http://gerrit.cloudera.org:8080/#/c/19002/7/common/thrift/JniCatalog.thrift File common/thrift/JniCatalog.thrift: http://gerrit.cloudera.org:8080/#/c/19002/7/common/thrift/JniCatalog.thrift@416 PS7, Line 416: TAlterTableExecuteExpireSnaps > I think this should be Thanks, that is better http://gerrit.cloudera.org:8080/#/c/19002/7/common/thrift/JniCatalog.thrift@505 PS7, Line 505: // Parameters for ALTER TABLE EXECUTE operations > nit: // Parameters for ALTER TABLE EXECUTE ... operations. Thanks http://gerrit.cloudera.org:8080/#/c/19002/7/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/7/fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteRollbackStmt.java@74 PS7, Line 74: USAGE + " <expression> must be a constant expression: EXECUTE " + toSql()); : } : if ((fnParamValue_ instanceof LiteralExpr) : && (fnParamValue_.getType().isIntegerType())) { : // Parameter is a snapshot id : kind_ = TRollbackType.VERSION_ID; : snapshotVersion_ = fnParamValue_.evalToInteger(analyzer, USAGE); : if (snapshotVersion_ < 0) { : throw new AnalysisException("Invalid version number has been given to " + USAGE : + ": " + snapshotVersion_); : } : LOG.debug(USAGE + " version: " + snapshotVersion_); : } else { : Expr timestampEpr = getParamConvertibleToTimestamp(); : if (timestampEpr != null) { : // Parameter is a timestamp. : kind_ = TRollbackType.TIME_ID; : try { : olderThanMillis_ = : ExprUtil.localTimestampT > The control flow is kinda messy here. Is being decimal important at all? Fa Thanks for the thoughtful comment. Yes this is better http://gerrit.cloudera.org:8080/#/c/19002/7/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/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1338 PS7, Line 1338: > Shouldn't be this method be modified to alterTableExecuteExpireSnapshots to yes, thanks http://gerrit.cloudera.org:8080/#/c/19002/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1343 PS7, Line 1343: > nit: Maybe this is a bit more telling for debugging purposes. That is better thanks http://gerrit.cloudera.org:8080/#/c/19002/7/testdata/workloads/functional-query/queries/QueryTest/iceberg-rollback.test File testdata/workloads/functional-query/queries/QueryTest/iceberg-rollback.test: http://gerrit.cloudera.org:8080/#/c/19002/7/testdata/workloads/functional-query/queries/QueryTest/iceberg-rollback.test@18 PS7, Line 18: ROLLBACK('1111') : ---- CATCH : An invalid TIMESTAMP express > This is what I meant with weird error message, if I want to rollback to my Thanks I have made it understandable by humans I hope http://gerrit.cloudera.org:8080/#/c/19002/7/tests/common/iceberg_test_suite.py File tests/common/iceberg_test_suite.py: http://gerrit.cloudera.org:8080/#/c/19002/7/tests/common/iceberg_test_suite.py@105 PS7, Line 105: """Construct a snapshot from a > Could you add here the expected format of the raw string in a comment? Done -- 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: 8 Gerrit-Owner: Andrew Sherman <[email protected]> Gerrit-Reviewer: Andrew Sherman <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Gergely Fürnstáhl <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tamas Mate <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Wed, 05 Oct 2022 17:48:45 +0000 Gerrit-HasComments: Yes
