Matthew Jacobs has posted comments on this change. Change subject: IMPALA-3719: Simplify CREATE TABLE statements with Kudu tables ......................................................................
Patch Set 5: (4 comments) http://gerrit.cloudera.org:8080/#/c/4414/4/fe/src/main/java/org/apache/impala/catalog/KuduTable.java File fe/src/main/java/org/apache/impala/catalog/KuduTable.java: PS4, Line 175: } > Done While it does seem related, does it really mean the same thing? Either way, we have to be sure we're handling it correctly everywhere. E.g. does Table.getClusteringColumns(), getNonClusteringColumns(), work as expected? They also get stored in the thrift TTable differently, see Table.java: // Clustering columns come first. if (i < numClusteringCols_) { table.addToClustering_columns(colDesc); } else { table.addToColumns(colDesc); } Further, Table.getNumClusteringCols() exposes this and it is used in a lot of places. E.g. TupleDescriptor.java: public boolean hasClusteringColsOnly() { Table table = getTable(); if (!(table instanceof HdfsTable) || table.getNumClusteringCols() == 0) return false; ... There are a ton of other places to check. PS4, Line 226: private void loadDistributeByParams(org.apache.kudu.client.KuduTable kuduTable) { : Preconditions > cols is a reference to msTable cols. We clear them here and reload them fro I see. yes, a brief comment would be helpful. http://gerrit.cloudera.org:8080/#/c/4414/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: PS5, Line 1484: // Add the table to the catalog cache : Table newTbl = catalog_.addTable(newTable.getDbName(), newTable.getTableName()); : addTableToCatalogUpdate(newTbl, response.result); If something here throws (e.g. addTable() looks like it could throw), we probably need to roll back the hiveClient.createTable() call, right? http://gerrit.cloudera.org:8080/#/c/4414/4/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java: PS4, Line 135: tableOpts.setRangePartitionColumns( : distParam.getBy_range_param().getColumns()); : > Unfortunately it is. I spoke to Dan (from Kudu team) about it. If the user Thanks, I followed up with an e-mail to Dan to make sure I understand the Kudu behavior. -- To view, visit http://gerrit.cloudera.org:8080/4414 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7b9d51b2720ab57649abdb7d5c710ea04ff50dc1 Gerrit-PatchSet: 5 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Dimitris Tsirogiannis <dtsirogian...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogian...@cloudera.com> Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com> Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com> Gerrit-Reviewer: Michael Brown <mi...@cloudera.com> Gerrit-HasComments: Yes
