Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10656 )
Change subject: KUDU-2459: add placeholder names to some CREATE TABLE statements ...................................................................... Patch Set 12: (7 comments) I filed KUDU-2523 for the test failure, which looks unrelated to your patch. http://gerrit.cloudera.org:8080/#/c/10656/12//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10656/12//COMMIT_MSG@12 PS12, Line 12: tablename Across this commit message (and the code), you're writing both "table name" and "tablename". Can you pick one term and use it consistently? http://gerrit.cloudera.org:8080/#/c/10656/12//COMMIT_MSG@17 PS12, Line 17: drive derive http://gerrit.cloudera.org:8080/#/c/10656/12//COMMIT_MSG@19 PS12, Line 19: We Nit: space before "We" http://gerrit.cloudera.org:8080/#/c/10656/12//COMMIT_MSG@21 PS12, Line 21: invalid tablenames have been identified and updated by a placeholder : tablename for end user to replace with a valid tablename. The rules to Nit: you have trailing whitespace on these two lines. http://gerrit.cloudera.org:8080/#/c/10656/5/src/kudu/master/master_path_handlers.cc File src/kudu/master/master_path_handlers.cc: http://gerrit.cloudera.org:8080/#/c/10656/5/src/kudu/master/master_path_handlers.cc@269 PS5, Line 269: if (orig_name.length() <= 128) { > Combining these if-conditions exceeds the total code line length requiremen I don't think a combined if statement with line breaks is so bad: if (!orig_name.empty() && orig_name.length() <= 128 && ascii_isalpha(orig_name[0]) && find_if(orig_name.begin(), orig_name.end(), [](char c) { return !(isalnum(c) || (c == '_')); }) == orig_name.end()) { impala_name = orig_name; } http://gerrit.cloudera.org:8080/#/c/10656/12/src/kudu/master/master_path_handlers.cc File src/kudu/master/master_path_handlers.cc: http://gerrit.cloudera.org:8080/#/c/10656/12/src/kudu/master/master_path_handlers.cc@268 PS12, Line 268: //Ensuring non-zero-length tablename Style nit: format single-line comments as "// foo bar" not "//foo bar". http://gerrit.cloudera.org:8080/#/c/10656/12/src/kudu/master/master_path_handlers.cc@270 PS12, Line 270: //Ensuring that the tablename length does not exceed 128 characters I think these comments would be more readable if grouped into a single block at the beginning. Something like: // Not all Kudu table names are also valid Impala identifiers. We need to replace such names // with a placeholder because ... // // Valid Impala identifiers conform to the following rules: // 1. Must not be empty. // 2. Length must not exceed 128 characters. // 3. First character must be alphabetic. // 4. Every character must be alphanumeric or an underscore. -- To view, visit http://gerrit.cloudera.org:8080/10656 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If9e5242318589452a0507d3973b9be3e6a186a69 Gerrit-Change-Number: 10656 Gerrit-PatchSet: 12 Gerrit-Owner: Shriya Gupta <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Jean-Daniel Cryans <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Shriya Gupta <[email protected]> Gerrit-Reviewer: Thomas Marshall <[email protected]> Gerrit-Comment-Date: Wed, 01 Aug 2018 17:13:45 +0000 Gerrit-HasComments: Yes
