Tim Armstrong 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 1: (9 comments) I think this is pretty good, some cleanup and some tests suggested. 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 if it doesn't have a transaction context. Might be worth explicitly saying that the client is responsible for calling releaseTableLock()? http://gerrit.cloudera.org:8080/#/c/14071/1/fe/src/main/java/org/apache/impala/catalog/Catalog.java@687 PS1, Line 687: 0 for standalone locks. I think we should hide this from callers. How about we add two public variants of lockTable() - lockTableInTransaction() that takes a Transaction argument, and lockTableStandalone() that doesn't take a txnId argument. They could both use this function as the private implementation. It would also be nice to use the auto-close pattern for standalone locks, but that's unrelated to this change. 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 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? 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: hmsPartitons typo: hmsPartitions http://gerrit.cloudera.org:8080/#/c/14071/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1832 PS1, Line 1832: if (!hmsPartitons.isEmpty()) { Can you comment what this special case is required for? Do we have a test case that exercises the case when we skip unsetting the stats. 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 somewhat interesting case. 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. truncating a truncated table doesn't break anything). http://gerrit.cloudera.org:8080/#/c/14071/1/testdata/workloads/functional-query/queries/QueryTest/acid-truncate.test@90 PS1, Line 90: truncate table pt; Do we need to confirm that the stats are correct? -- 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: 1 Gerrit-Owner: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Reviewer: Dinesh Garg (430) Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Yongzhi Chen <yc...@cloudera.com> Gerrit-Comment-Date: Thu, 15 Aug 2019 21:08:37 +0000 Gerrit-HasComments: Yes
