Gergely Fürnstáhl 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 7: (7 comments) Generally looks good to me, added a few small 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: TExecuteExpireSnapshotsParams I think this should be TAlterTableExecuteExpireSnapshotsParams http://gerrit.cloudera.org:8080/#/c/19002/7/common/thrift/JniCatalog.thrift@505 PS7, Line 505: // Parameters for ALTER TABLE EXECUTE EXPIRE_SNAPSHOTS nit: // Parameters for ALTER TABLE EXECUTE ... operations. 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: if (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_); : return; : } else if (fnParamValue_.getType().isDecimal()) { : throw new AnalysisException(USAGE : + " <expression> must be an integer type or a timestamp, but is '" : + fnParamValue_.getType() + "': EXECUTE " + toSql()); : } : } : : // Parameter is a timestamp : kind_ = TRollbackType.TIME_ID; : analyzeOlderThan(analyzer, USAGE); The control flow is kinda messy here. Is being decimal important at all? Falling back to parsing it as timestamp could produce hard to understand error messages too. Maybe something like this would be easier to follow/maintain/extend it later: if(literal && integer) { ... } else if(covertibleToTimestamp()) { ... } // this could be extracted from analyzeOlderThan else { ... } // not supported, must be integer or timestamp 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: alterTableExecute Shouldn't be this method be modified to alterTableExecuteExpireSnapshots too? http://gerrit.cloudera.org:8080/#/c/19002/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1343 PS7, Line 1343: Alter table execute has new format. nit: Maybe this is a bit more telling for debugging purposes. "Alter table execute statement is not implemented." 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 : Invalid TIMESTAMP expression This is what I meant with weird error message, if I want to rollback to my id=1111 snapshot, but Impala is telling its not a valid timestamp, hard to find that the mistake is the added ' '. 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: parts = raw_string.split("\t") Could you add here the expected format of the raw string in a comment? -- 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: 7 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: Mon, 03 Oct 2022 13:53:50 +0000 Gerrit-HasComments: Yes
