Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/14071 )
Change subject: IMPALA-8793: Implement TRUNCATE for insert-only ACID tables ...................................................................... Patch Set 2: (12 comments) http://gerrit.cloudera.org:8080/#/c/14071/1/fe/src/main/java/org/apache/impala/analysis/TruncateStmt.java File fe/src/main/java/org/apache/impala/analysis/TruncateStmt.java: http://gerrit.cloudera.org:8080/#/c/14071/1/fe/src/main/java/org/apache/impala/analysis/TruncateStmt.java@71 PS1, Line 71: } > I think this line can be removed, the check on line 70 already blocks the F Done http://gerrit.cloudera.org:8080/#/c/14071/1/fe/src/main/java/org/apache/impala/catalog/Catalog.java File fe/src/main/java/org/apache/impala/catalog/Catalog.java: http://gerrit.cloudera.org:8080/#/c/14071/1/fe/src/main/java/org/apache/impala/catalog/Catalog.java@684 PS1, Line 684: * heartbeating the lock. This function is for locks that doesn't belong to a > Might be worth explicitly saying that the client is responsible for calling Done http://gerrit.cloudera.org:8080/#/c/14071/1/fe/src/main/java/org/apache/impala/catalog/Catalog.java@687 PS1, Line 687: he particular table is. > I think we should hide this from callers. How about we add two public varia Modified the lockTable() interface as suggested. Yeah it would be useful to create a LockGuard as well, but the code structure is a bit different for drop table and I don't want to mess it up. But we plan to do some cleanup soon. http://gerrit.cloudera.org:8080/#/c/14071/1/fe/src/main/java/org/apache/impala/catalog/Catalog.java@704 PS1, Line 704: */ > Is there a corresponding remove lock part? For truncate, the lock is removed when the transaction is aborted/committed. DROP table uses this method also, it removes the lock manually because DROP doesn't need to run in transaction. http://gerrit.cloudera.org:8080/#/c/14071/1/fe/src/main/java/org/apache/impala/catalog/Transaction.java File fe/src/main/java/org/apache/impala/catalog/Transaction.java: http://gerrit.cloudera.org:8080/#/c/14071/1/fe/src/main/java/org/apache/impala/catalog/Transaction.java@17 PS1, Line 17: public class Transaction implements AutoCloseable { > This is really nice Thanks! http://gerrit.cloudera.org:8080/#/c/14071/1/fe/src/main/java/org/apache/impala/catalog/Transaction.java@37 PS1, Line 37: public void commit() throws TransactionException { > Precondition check that transactionId > 0? Done http://gerrit.cloudera.org:8080/#/c/14071/1/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/14071/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1817 PS1, Line 1817: > typo: hmsPartitions Done http://gerrit.cloudera.org:8080/#/c/14071/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1832 PS1, Line 1832: hmsPartitions.add(hmsPart); > Can you comment what this special case is required for? Do we have a test c It's skipped for non-partitioned tables. Extended the comment about it. The tests check column stats-property removal for both partitioned and non-partitioned tables. http://gerrit.cloudera.org:8080/#/c/14071/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1854 PS1, Line 1854: * @param writeId the write id of the new base directory. > Could you comment a little more on why a transactional table does not remov Extended truncateTransactionalTable()'s comment. http://gerrit.cloudera.org:8080/#/c/14071/1/testdata/workloads/functional-query/queries/QueryTest/acid-truncate.test File testdata/workloads/functional-query/queries/QueryTest/acid-truncate.test: PS1: > Can you also test truncating a completely empty table? I think that's a som Added a TRUNCATE stmt just after table creation. http://gerrit.cloudera.org:8080/#/c/14071/1/testdata/workloads/functional-query/queries/QueryTest/acid-truncate.test@40 PS1, Line 40: ==== > Can you truncate the table again here, to confirm that it's idempotent (i.e Sure, done. http://gerrit.cloudera.org:8080/#/c/14071/1/testdata/workloads/functional-query/queries/QueryTest/acid-truncate.test@90 PS1, Line 90: ==== > Do we need to confirm that the stats are correct? Unfortunately for partitioned tables we can only check it implicitly. We truncate the table here, then in test_acid.py::test_acid_truncate() we issue a count(*) query towards Hive and if it sees 0 rows then it means we successfully invalidated the statistics. Other examples for this are the test_*_statschg methods in test_acid.py. -- To view, visit http://gerrit.cloudera.org:8080/14071 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic749b7f27da157e1c0ebf9b7e9b6ee09afad122a Gerrit-Change-Number: 14071 Gerrit-PatchSet: 2 Gerrit-Owner: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: Dinesh Garg (430) Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Yongzhi Chen <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Fri, 16 Aug 2019 14:30:44 +0000 Gerrit-HasComments: Yes
