Alex Behm has posted comments on this change. Change subject: IMPALA-3719: Simplify CREATE TABLE statements with Kudu tables ......................................................................
Patch Set 5: (8 comments) Responses to comments. Starting next round on code. http://gerrit.cloudera.org:8080/#/c/4414/5/fe/src/main/java/org/apache/impala/catalog/KuduTable.java File fe/src/main/java/org/apache/impala/catalog/KuduTable.java: Line 195: msClient.alter_table(msTable_.getDbName(), msTable_.getTableName(), msTable_); > Good point. We can potentially avoid this by checking if there has been som Right, most of the time there will be no changes. I'm not only worried about the perf, but all the extra weird states we can be in when doing this extra RPC to modify the HMS. Line 216: String colName = ToSqlUtils.getIdentSql(colSchema.getName()); > Hm, can you explain why this is the case given that we will use this name t We don't parse the column names in the HMS backend table and neither does Hive. The backticks are only needed to tell our parser to interpret a token as an identifier, but the parser is never invoked for column names from the HMS. I recommend trying this out with some Impala keywords to see what happens. Line 223: numClusteringCols_ = primaryKeyColumnNames_.size(); > Actually, I removed this and left a TODO. There are a few places that inter Yea, there's definitely some inconsistency here. 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: Line 1151: // the metadata must be fetched from the Hive Metastore. > In order to drop a table from Kudu we need the Kudu table name which is sto Makes sense now that you've explained, please leave a brief comment. Line 1172: if (!KuduTable.isKuduTable(msTable)) continue; > Well, it's an easy check we can do to avoid RPC calls to Kudu for dropping Got it, thanks! http://gerrit.cloudera.org:8080/#/c/4414/5/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java: Line 85: kudu.createTable(kuduTableName, schema, tableOpts); > I see what you're saying but I don't think this will work. We need to be ab Agree. Can you file a JIRA against Kudu? Thanks! Line 178: if (kudu.tableExists(tableName)) { > I get it. See my comment above and let me know what you think. I agree with you. Line 214: String colName = ToSqlUtils.getIdentSql(colSchema.getName()); > I'd appreciate it if you could explain why is it wrong to use this here and Explained in other comment, let me know if still unclear :) -- 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
