Tamas Mate has posted comments on this change. ( http://gerrit.cloudera.org:8080/18688 )
Change subject: IMPALA-11362: Add expire snapshots functionality for Iceberg tables ...................................................................... Patch Set 4: (9 comments) Thank you for the review Zoli, updated the change. http://gerrit.cloudera.org:8080/#/c/18688/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18688/3//COMMIT_MSG@12 PS3, Line 12: a safe : solution > nit: safe solutions / a safe solution Done http://gerrit.cloudera.org:8080/#/c/18688/3/fe/src/main/cup/sql-parser.cup File fe/src/main/cup/sql-parser.cup: http://gerrit.cloudera.org:8080/#/c/18688/3/fe/src/main/cup/sql-parser.cup@1315 PS3, Line 1315: RESULT = new AlterTableExecuteStmt(table, expr); > Do we need this comment? We don't, removed. http://gerrit.cloudera.org:8080/#/c/18688/3/fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteStmt.java File fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteStmt.java: http://gerrit.cloudera.org:8080/#/c/18688/3/fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteStmt.java@37 PS3, Line 37: : * TableProperties.MIN_SNAPSHOTS_TO_KEEP table property manages how many snapshots : * should be retained even when all snapshots are selected by expire > I'm not sure we need this part. I think we only need to explain the role of Done http://gerrit.cloudera.org:8080/#/c/18688/3/fe/src/main/java/org/apache/impala/analysis/AlterTableExecuteStmt.java@46 PS3, Line 46: funct > nit: I think we use upper case letters for constants. Also, this could be s Done http://gerrit.cloudera.org:8080/#/c/18688/3/tests/common/iceberg_test_suite.py File tests/common/iceberg_test_suite.py: http://gerrit.cloudera.org:8080/#/c/18688/3/tests/common/iceberg_test_suite.py@26 PS3, Line 26: cls, > nit: class methods take a Class parameter which is usually named 'cls', not Done http://gerrit.cloudera.org:8080/#/c/18688/3/tests/common/iceberg_test_suite.py@40 PS3, Line 40: expect_num_snapshot > maybe: expect_num_snapshots_from Done http://gerrit.cloudera.org:8080/#/c/18688/3/tests/query_test/test_iceberg.py File tests/query_test/test_iceberg.py: http://gerrit.cloudera.org:8080/#/c/18688/3/tests/query_test/test_iceberg.py@87 PS3, Line 87: ad_client.execut > nit: we just need execute as we don't use the return value. Done http://gerrit.cloudera.org:8080/#/c/18688/3/tests/query_test/test_iceberg.py@91 PS3, Line 91: interval > nit: interval Done http://gerrit.cloudera.org:8080/#/c/18688/3/tests/query_test/test_iceberg.py@95 PS3, Line 95: but retain 1 > We should also add a test which sets table property TableProperties.MIN_SNA Done -- To view, visit http://gerrit.cloudera.org:8080/18688 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ideffee4964c18c85ca745bfb4eca08ec362416f3 Gerrit-Change-Number: 18688 Gerrit-PatchSet: 4 Gerrit-Owner: Tamas Mate <tma...@apache.org> 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: Tue, 12 Jul 2022 10:55:30 +0000 Gerrit-HasComments: Yes
