Sahil Takiar has posted comments on this change. ( http://gerrit.cloudera.org:8080/12179 )
Change subject: IMPALA-7640: RENAME on managed Kudu table should rename Kudu table ...................................................................... Patch Set 1: (3 comments) http://gerrit.cloudera.org:8080/#/c/12179/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/12179/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2378 PS1, Line 2378: if (oldTbl instanceof KuduTable && !Table.isExternalTable(msTbl)) { > Can you add a precondition check for (KuduTable.isKuduTable(msTbl))? Done http://gerrit.cloudera.org:8080/#/c/12179/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2586 PS1, Line 2586: if (KuduTable.isKuduTable(msTbl)) { > Can this code now use renameKuduTable()? I considered that, but doing so would essentially make renameKuduTable() a wrapper around KuduCatalogOpExecutor.renameTable() The way KuduCatalogOpExecutor.renameTable is executed here vs. in renameKuduTable() is different enough that making them use a common method doesn't help much. The differences are that (1) we use properties.get(KuduTable.KEY_TABLE_NAME) to get the table name vs KuduUtil.getDefaultCreateKuduTableName, (2) all the altered properties have to be added to the msTbl before KuduCatalogOpExecutor.validateKuduTblExists is invoked - it seems that the call to validateKuduTblExists validates the table is accessible *and* that the new properties are valid. http://gerrit.cloudera.org:8080/#/c/12179/1/testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test File testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test: http://gerrit.cloudera.org:8080/#/c/12179/1/testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test@370 PS1, Line 370: alter table tbl_to_alter rename to kudu_tbl_to_alter > Can you add a test that makes sure that the underlying table got renamed su This query + the "create external table external_tbl stored as kudu..." one below already cover this. Together they validate the the table was renamed. I added another test to make sure that the original table does not exist. -- To view, visit http://gerrit.cloudera.org:8080/12179 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I77e7583ce93cba8f6e743c4bedd9900ae1fae081 Gerrit-Change-Number: 12179 Gerrit-PatchSet: 1 Gerrit-Owner: Sahil Takiar <stak...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Sahil Takiar <stak...@cloudera.com> Gerrit-Comment-Date: Wed, 23 Jan 2019 23:07:23 +0000 Gerrit-HasComments: Yes
