[kudu-CR] KUDU-2459: add placeholder names to some CREATE TABLE statements
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/10656 ) Change subject: KUDU-2459: add placeholder names to some CREATE TABLE statements .. KUDU-2459: add placeholder names to some CREATE TABLE statements Under the Tables section of Kudu Web UI, for a selected table, the table metrics display a CREATE TABLE statement that can be run to make Impala cognizant of that table. However, in generation of this statement, the tablename tries to match the original Kudu tablename which may not always be acceptable as a tablename for Impala. For example, Kudu accepts dot in tablename, Impala does not. The CREATE TABLE statement thus throws an invalid tablename error in Impala. We considered trying to derive an Impala-conforming name from the Kudu- supplied tablename but that could result in surprising/unintuitive results. We also want to leverage the current functionality of auto- generated statement where the name is valid. Therefore with this update, invalid tablenames have been identified and updated by a placeholder tablename for end user to replace with a valid tablename. The rules to check Impala conformity are below -- - The minimum length of an identifier is 1 character. - The maximum length of an identifier is currently 128 characters. - An identifier must start with an alphabetic character. The remainder can contain any combination of alphanumeric characters and underscores. Quoting the identifier with backticks has no effect on the allowed characters in the name. - An identifier can contain only ASCII characters. Change-Id: If9e5242318589452a0507d3973b9be3e6a186a69 Reviewed-on: http://gerrit.cloudera.org:8080/10656 Reviewed-by: Adar Dembo Tested-by: Kudu Jenkins --- M src/kudu/master/master_path_handlers.cc M www/table.mustache 2 files changed, 28 insertions(+), 1 deletion(-) Approvals: Adar Dembo: Looks good to me, approved Kudu Jenkins: Verified -- 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: merged Gerrit-Change-Id: If9e5242318589452a0507d3973b9be3e6a186a69 Gerrit-Change-Number: 10656 Gerrit-PatchSet: 17 Gerrit-Owner: Shriya Gupta Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Shriya Gupta Gerrit-Reviewer: Thomas Marshall
[kudu-CR] KUDU-2459: add placeholder names to some CREATE TABLE statements
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 16: Code-Review+2 -- 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: 16 Gerrit-Owner: Shriya Gupta Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Shriya Gupta Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Wed, 01 Aug 2018 20:40:11 + Gerrit-HasComments: No
[kudu-CR] KUDU-2459: add placeholder names to some CREATE TABLE statements
Shriya Gupta 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 16: (2 comments) http://gerrit.cloudera.org:8080/#/c/10656/14/src/kudu/master/master_path_handlers.cc File src/kudu/master/master_path_handlers.cc: http://gerrit.cloudera.org:8080/#/c/10656/14/src/kudu/master/master_path_handlers.cc@265 PS14, Line 265: : // Not all Kudu tablenames are also valid Impala identifiers. We need to : // replace such names with a placeholder when they are used as Impala > These lines look like they're too long: please wrap at 80 characters. Done http://gerrit.cloudera.org:8080/#/c/10656/14/src/kudu/master/master_path_handlers.cc@273 PS14, Line 273: // 4. Every character must be alphanumeric or an underscore. > Style nit: when you want to emphasize that a comment applies to a particula That's a good pointer. Done. -- 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: 16 Gerrit-Owner: Shriya Gupta Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Shriya Gupta Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Wed, 01 Aug 2018 20:08:11 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2459: add placeholder names to some CREATE TABLE statements
Hello Thomas Marshall, Jean-Daniel Cryans, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10656 to look at the new patch set (#16). Change subject: KUDU-2459: add placeholder names to some CREATE TABLE statements .. KUDU-2459: add placeholder names to some CREATE TABLE statements Under the Tables section of Kudu Web UI, for a selected table, the table metrics display a CREATE TABLE statement that can be run to make Impala cognizant of that table. However, in generation of this statement, the tablename tries to match the original Kudu tablename which may not always be acceptable as a tablename for Impala. For example, Kudu accepts dot in tablename, Impala does not. The CREATE TABLE statement thus throws an invalid tablename error in Impala. We considered trying to derive an Impala-conforming name from the Kudu- supplied tablename but that could result in surprising/unintuitive results. We also want to leverage the current functionality of auto- generated statement where the name is valid. Therefore with this update, invalid tablenames have been identified and updated by a placeholder tablename for end user to replace with a valid tablename. The rules to check Impala conformity are below -- - The minimum length of an identifier is 1 character. - The maximum length of an identifier is currently 128 characters. - An identifier must start with an alphabetic character. The remainder can contain any combination of alphanumeric characters and underscores. Quoting the identifier with backticks has no effect on the allowed characters in the name. - An identifier can contain only ASCII characters. Change-Id: If9e5242318589452a0507d3973b9be3e6a186a69 --- M src/kudu/master/master_path_handlers.cc M www/table.mustache 2 files changed, 28 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/10656/16 -- 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: newpatchset Gerrit-Change-Id: If9e5242318589452a0507d3973b9be3e6a186a69 Gerrit-Change-Number: 10656 Gerrit-PatchSet: 16 Gerrit-Owner: Shriya Gupta Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Shriya Gupta Gerrit-Reviewer: Thomas Marshall
[kudu-CR] KUDU-2459: add placeholder names to some CREATE TABLE statements
Hello Thomas Marshall, Jean-Daniel Cryans, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10656 to look at the new patch set (#15). Change subject: KUDU-2459: add placeholder names to some CREATE TABLE statements .. KUDU-2459: add placeholder names to some CREATE TABLE statements Under the Tables section of Kudu Web UI, for a selected table, the table metrics display a CREATE TABLE statement that can be run to make Impala cognizant of that table. However, in generation of this statement, the tablename tries to match the original Kudu tablename which may not always be acceptable as a tablename for Impala. For example, Kudu accepts dot in tablename, Impala does not. The CREATE TABLE statement thus throws an invalid tablename error in Impala. We considered trying to derive an Impala-conforming name from the Kudu- supplied tablename but that could result in surprising/unintuitive results. We also want to leverage the current functionality of auto- generated statement where the name is valid. Therefore with this update, invalid tablenames have been identified and updated by a placeholder tablename for end user to replace with a valid tablename. The rules to check Impala conformity are below -- - The minimum length of an identifier is 1 character. - The maximum length of an identifier is currently 128 characters. - An identifier must start with an alphabetic character. The remainder can contain any combination of alphanumeric characters and underscores. Quoting the identifier with backticks has no effect on the allowed characters in the name. - An identifier can contain only ASCII characters. Change-Id: If9e5242318589452a0507d3973b9be3e6a186a69 --- M src/kudu/master/master_path_handlers.cc M www/table.mustache 2 files changed, 25 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/10656/15 -- 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: newpatchset Gerrit-Change-Id: If9e5242318589452a0507d3973b9be3e6a186a69 Gerrit-Change-Number: 10656 Gerrit-PatchSet: 15 Gerrit-Owner: Shriya Gupta Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Shriya Gupta Gerrit-Reviewer: Thomas Marshall
[kudu-CR] KUDU-2459: add placeholder names to some CREATE TABLE statements
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 14: (2 comments) http://gerrit.cloudera.org:8080/#/c/10656/14/src/kudu/master/master_path_handlers.cc File src/kudu/master/master_path_handlers.cc: http://gerrit.cloudera.org:8080/#/c/10656/14/src/kudu/master/master_path_handlers.cc@265 PS14, Line 265: // Not all Kudu tablenames are also valid Impala identifiers. We need to replace such names : // with a placeholder when they are used as Impala identifiers, for example as Impala tablename : // in the Kudu Web UI. Valid Impala identifiers conform to the following rules: These lines look like they're too long: please wrap at 80 characters. http://gerrit.cloudera.org:8080/#/c/10656/14/src/kudu/master/master_path_handlers.cc@273 PS14, Line 273: string impala_name = ""; Style nit: when you want to emphasize that a comment applies to a particular block of code, format it like this: What's important here? That the comment and the block of code aren't separated by an empty line, and that the two are separated from the code before and after with empty lines. This helps emphasize that the comment and the code should be considered together. -- 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: 14 Gerrit-Owner: Shriya Gupta Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Shriya Gupta Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Wed, 01 Aug 2018 18:58:08 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2459: add placeholder names to some CREATE TABLE statements
Hello Thomas Marshall, Jean-Daniel Cryans, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10656 to look at the new patch set (#14). Change subject: KUDU-2459: add placeholder names to some CREATE TABLE statements .. KUDU-2459: add placeholder names to some CREATE TABLE statements Under the Tables section of Kudu Web UI, for a selected table, the table metrics display a CREATE TABLE statement that can be run to make Impala cognizant of that table. However, in generation of this statement, the tablename tries to match the original Kudu tablename which may not always be acceptable as a tablename for Impala. For example, Kudu accepts dot in tablename, Impala does not. The CREATE TABLE statement thus throws an invalid tablename error in Impala. We considered trying to derive an Impala-conforming name from the Kudu- supplied tablename but that could result in surprising/unintuitive results. We also want to leverage the current functionality of auto- generated statement where the name is valid. Therefore with this update, invalid tablenames have been identified and updated by a placeholder tablename for end user to replace with a valid tablename. The rules to check Impala conformity are below -- - The minimum length of an identifier is 1 character. - The maximum length of an identifier is currently 128 characters. - An identifier must start with an alphabetic character. The remainder can contain any combination of alphanumeric characters and underscores. Quoting the identifier with backticks has no effect on the allowed characters in the name. - An identifier can contain only ASCII characters. Change-Id: If9e5242318589452a0507d3973b9be3e6a186a69 --- M src/kudu/master/master_path_handlers.cc M www/table.mustache 2 files changed, 22 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/10656/14 -- 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: newpatchset Gerrit-Change-Id: If9e5242318589452a0507d3973b9be3e6a186a69 Gerrit-Change-Number: 10656 Gerrit-PatchSet: 14 Gerrit-Owner: Shriya Gupta Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Shriya Gupta Gerrit-Reviewer: Thomas Marshall
[kudu-CR] KUDU-2459: add placeholder names to some CREATE TABLE statements
Shriya Gupta 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 13: (7 comments) > (7 comments) > > I filed KUDU-2523 for the test failure, which looks unrelated to > your patch. Yep, thank you, I wasn't sure why a commit message update on a successful build would warrant/fail a debug build test. I have made the other updates too. 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: ablename > Across this commit message (and the code), you're writing both "table name" Done http://gerrit.cloudera.org:8080/#/c/10656/12//COMMIT_MSG@17 PS12, Line 17: deriv > derive Done http://gerrit.cloudera.org:8080/#/c/10656/12//COMMIT_MSG@19 PS12, Line 19: W > Nit: space before "We" Done 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. Done 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: // 2. Length must not exceed 128 characters. > I don't think a combined if statement with line breaks is so bad: Done 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: // 1. Must not be empty. > Style nit: format single-line comments as "// foo bar" not "//foo bar". Done http://gerrit.cloudera.org:8080/#/c/10656/12/src/kudu/master/master_path_handlers.cc@270 PS12, Line 270: // 3. First character must be alphabetic. > I think these comments would be more readable if grouped into a single bloc Done -- 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: 13 Gerrit-Owner: Shriya Gupta Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Shriya Gupta Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Wed, 01 Aug 2018 18:16:20 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2459: add placeholder names to some CREATE TABLE statements
Hello Thomas Marshall, Jean-Daniel Cryans, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10656 to look at the new patch set (#13). Change subject: KUDU-2459: add placeholder names to some CREATE TABLE statements .. KUDU-2459: add placeholder names to some CREATE TABLE statements Under the Tables section of Kudu Web UI, for a selected table, the table metrics display a CREATE TABLE statement that can be run to make Impala cognizant of that table. However, in generation of this statement, the tablename tries to match the original Kudu tablename which may not always be acceptable as a tablename for Impala. For example, Kudu accepts dot in tablename, Impala does not. The CREATE TABLE statement thus throws an invalid tablename error in Impala. We considered trying to derive an Impala-conforming name from the Kudu- supplied tablename but that could result in surprising/unintuitive results. We also want to leverage the current functionality of auto- generated statement where the name is valid. Therefore with this update, invalid tablenames have been identified and updated by a placeholder tablename for end user to replace with a valid tablename. The rules to check Impala conformity are below -- - The minimum length of an identifier is 1 character. - The maximum length of an identifier is currently 128 characters. - An identifier must start with an alphabetic character. The remainder can contain any combination of alphanumeric characters and underscores. Quoting the identifier with backticks has no effect on the allowed characters in the name. - An identifier can contain only ASCII characters. Change-Id: If9e5242318589452a0507d3973b9be3e6a186a69 --- M src/kudu/master/master_path_handlers.cc M www/table.mustache 2 files changed, 22 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/10656/13 -- 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: newpatchset Gerrit-Change-Id: If9e5242318589452a0507d3973b9be3e6a186a69 Gerrit-Change-Number: 10656 Gerrit-PatchSet: 13 Gerrit-Owner: Shriya Gupta Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Shriya Gupta Gerrit-Reviewer: Thomas Marshall
[kudu-CR] KUDU-2459: add placeholder names to some CREATE TABLE statements
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
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Shriya Gupta
Gerrit-Reviewer: Thomas Marshall
Gerrit-Comment-Date: Wed, 01 Aug 2018 17:13:45 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2459: add placeholder names to some CREATE TABLE statements
Hello Thomas Marshall, Jean-Daniel Cryans, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10656 to look at the new patch set (#12). Change subject: KUDU-2459: add placeholder names to some CREATE TABLE statements .. KUDU-2459: add placeholder names to some CREATE TABLE statements Under the Tables section of Kudu Web UI, for a selected table, the table metrics display a CREATE TABLE statement that can be run to make Impala cognizant of that table. However, in generation of this statement, the table name tries to match the original Kudu tablename which may not always be acceptable as a tablename for Impala. For example, Kudu accepts dot in tablename, Impala does not. The CREATE TABLE statement thus throws an invalid tablename error in Impala. We considered trying to drive an Impala-conforming name from the Kudu- supplied tablename but that could result in surprising/unintuitive results.We also want to leverage the current functionality of auto- generated statement where the name is valid. Therefore with this update, invalid tablenames have been identified and updated by a placeholder tablename for end user to replace with a valid tablename. The rules to check Impala conformity are below -- - The minimum length of an identifier is 1 character. - The maximum length of an identifier is currently 128 characters. - An identifier must start with an alphabetic character. The remainder can contain any combination of alphanumeric characters and underscores. Quoting the identifier with backticks has no effect on the allowed characters in the name. - An identifier can contain only ASCII characters. Change-Id: If9e5242318589452a0507d3973b9be3e6a186a69 --- M src/kudu/master/master_path_handlers.cc M www/table.mustache 2 files changed, 24 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/10656/12 -- 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: newpatchset Gerrit-Change-Id: If9e5242318589452a0507d3973b9be3e6a186a69 Gerrit-Change-Number: 10656 Gerrit-PatchSet: 12 Gerrit-Owner: Shriya Gupta Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Shriya Gupta Gerrit-Reviewer: Thomas Marshall
[kudu-CR] KUDU-2459: add placeholder names to some CREATE TABLE statements
Shriya Gupta 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 11:
(8 comments)
Updates made per the comments.
http://gerrit.cloudera.org:8080/#/c/10656/5//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/10656/5//COMMIT_MSG@7
PS5, Line 7: KUDU-2459: add placeholder names to some CREATE TABLE statements
> We typically format commit messages as:
Done
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@266
PS5, Line 266: string orig_name = l.data().name();
: if (!orig_name.empty()) {
> Don't need "std::" prefixes; there's a "using std::string" at the top of th
Done
http://gerrit.cloudera.org:8080/#/c/10656/5/src/kudu/master/master_path_handlers.cc@268
PS5, Line 268: //Ensuring non-zero-length tablename
> Do !orig_name.empty() instead; it's more idiomatic.
Done
http://gerrit.cloudera.org:8080/#/c/10656/5/src/kudu/master/master_path_handlers.cc@269
PS5, Line 269: if (orig_name.length() <= 128) {
> Please combine these conditions rather than nesting more and more ifs toget
Combining these if-conditions exceeds the total code line length requirement. I
broke it into separate conditions for more readability purposes. I have added
code comments. I can still concatenate if you feel that's better, but with the
line breaks, the logic appears more complex to follow.
http://gerrit.cloudera.org:8080/#/c/10656/5/src/kudu/master/master_path_handlers.cc@270
PS5, Line 270: //Ensuring that the tablename length does not exceed 128
characters
> Add a space between the operator (>=) and each of the operands (orig_name[0
Done
http://gerrit.cloudera.org:8080/#/c/10656/5/src/kudu/master/master_path_handlers.cc@272
PS5, Line 272: //Ensuring first character of tablename is alphabetic
: if (find_if(orig_name.begin(), orig_name.end(),[](char
c) {
> Reformat this so it's more obvious that "return ..." is the body of a lambd
Done
http://gerrit.cloudera.org:8080/#/c/10656/5/src/kudu/master/master_path_handlers.cc@275
PS5, Line 275: //For every character in the name, it checks whether
the character is alphanumeric
> Won't this cause HandleTablePage to return early and not finish outputting
I have removed the return statement.
http://gerrit.cloudera.org:8080/#/c/10656/5/www/table.mustache
File www/table.mustache:
http://gerrit.cloudera.org:8080/#/c/10656/5/www/table.mustache@156
PS5, Line 156: name
> Does this need to be replaced with a placeholder too?
No, this is the actual kudu tablename, it needs to be carried here as is.
--
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: 11
Gerrit-Owner: Shriya Gupta
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Shriya Gupta
Gerrit-Reviewer: Thomas Marshall
Gerrit-Comment-Date: Wed, 01 Aug 2018 02:41:08 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2459: add placeholder names to some CREATE TABLE statements
Hello Thomas Marshall, Jean-Daniel Cryans, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10656 to look at the new patch set (#11). Change subject: KUDU-2459: add placeholder names to some CREATE TABLE statements .. KUDU-2459: add placeholder names to some CREATE TABLE statements Under the Tables section of Kudu Web UI, for a selected table, the table metrics display a CREATE TABLE statement that can be run to make Impala cognizant of that table. However, in generation of this statement, the table name tries to match the original Kudu tablename which may not always be acceptable as a tablename for Impala. For example, Kudu accepts dot in tablename, Impala does not. The CREATE TABLE statement thus throws an invalid tablename error in Impala. We considered trying to drive an Impala-conforming name from the Kudu- supplied tablename but that could result in surprising/unintuitive results.We also want to leverage the current functionality of auto- generated statement where the name is valid. Therefore, in this update, only invalidablenames have been updated by a placeholder tablename for end user to replace with a valid tablename. The rules to check Impala conformity are below -- - The minimum length of an identifier is 1 character. - The maximum length of an identifier is currently 128 characters. - An identifier must start with an alphabetic character. The remainder can contain any combination of alphanumeric characters and underscores. Quoting the identifier with backticks has no effect on the allowed characters in the name. - An identifier can contain only ASCII characters. Change-Id: If9e5242318589452a0507d3973b9be3e6a186a69 --- M src/kudu/master/master_path_handlers.cc M www/table.mustache 2 files changed, 24 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/10656/11 -- 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: newpatchset Gerrit-Change-Id: If9e5242318589452a0507d3973b9be3e6a186a69 Gerrit-Change-Number: 10656 Gerrit-PatchSet: 11 Gerrit-Owner: Shriya Gupta Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Shriya Gupta Gerrit-Reviewer: Thomas Marshall
[kudu-CR] KUDU-2459: add placeholder names to some CREATE TABLE statements
Hello Thomas Marshall, Jean-Daniel Cryans, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10656 to look at the new patch set (#10). Change subject: KUDU-2459: add placeholder names to some CREATE TABLE statements .. KUDU-2459: add placeholder names to some CREATE TABLE statements Under the Tables section of Kudu Web UI, for a selected table, the table metrics display a CREATE TABLE statement that can be run to make Impala cognizant of that table. However, in generation of this statement, the table name tries to match the original Kudu tablename which may not always be acceptable as a tablename for Impala. For example, Kudu accepts dot in tablename, Impala does not. The CREATE TABLE statement thus throws an invalid tablename error in Impala. We considered trying to drive an Impala-conforming name from the Kudu- supplied tablename but that could result in surprising/unintuitive results.We also want to leverage the current functionality of auto- generated statement where the name is valid. Therefore, in this update, only invalidablenames have been updated by a placeholder tablename for end user to replace with a valid tablename. The rules to check Impala conformity are below -- - The minimum length of an identifier is 1 character. - The maximum length of an identifier is currently 128 characters. - An identifier must start with an alphabetic character. The remainder can contain any combination of alphanumeric characters and underscores. Quoting the identifier with backticks has no effect on the allowed characters in the name. - An identifier can contain only ASCII characters. Change-Id: If9e5242318589452a0507d3973b9be3e6a186a69 --- M src/kudu/master/master_path_handlers.cc M www/table.mustache 2 files changed, 25 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/10656/10 -- 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: newpatchset Gerrit-Change-Id: If9e5242318589452a0507d3973b9be3e6a186a69 Gerrit-Change-Number: 10656 Gerrit-PatchSet: 10 Gerrit-Owner: Shriya Gupta Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Shriya Gupta Gerrit-Reviewer: Thomas Marshall
[kudu-CR] KUDU-2459: add placeholder names to some CREATE TABLE statements
Hello Thomas Marshall, Jean-Daniel Cryans, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10656 to look at the new patch set (#9). Change subject: KUDU-2459: add placeholder names to some CREATE TABLE statements .. KUDU-2459: add placeholder names to some CREATE TABLE statements Under the Tables section of Kudu Web UI, for a selected table, the table metrics display a CREATE TABLE statement that can be run to make Impala cognizant of that table. However, in generation of this statement, the table name tries to match the original Kudu tablename which may not always be acceptable as a tablename for Impala. For example, Kudu accepts dot in tablename, Impala does not. The CREATE TABLE statement thus throws an invalid tablename error in Impala. We considered trying to drive an Impala-conforming name from the Kudu- supplied tablename but that could result in surprising/unintuitive results.We also want to leverage the current functionality of auto- generated statement where the name is valid. Therefore, in this update, only invalidablenames have been updated by a placeholder tablename for end user to replace with a valid tablename. The rules to check Impala conformity are below -- - The minimum length of an identifier is 1 character. - The maximum length of an identifier is currently 128 characters. - An identifier must start with an alphabetic character. The remainder can contain any combination of alphanumeric characters and underscores. Quoting the identifier with backticks has no effect on the allowed characters in the name. - An identifier can contain only ASCII characters. Change-Id: If9e5242318589452a0507d3973b9be3e6a186a69 --- M src/kudu/master/master_path_handlers.cc M www/table.mustache 2 files changed, 25 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/10656/9 -- 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: newpatchset Gerrit-Change-Id: If9e5242318589452a0507d3973b9be3e6a186a69 Gerrit-Change-Number: 10656 Gerrit-PatchSet: 9 Gerrit-Owner: Shriya Gupta Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Shriya Gupta Gerrit-Reviewer: Thomas Marshall
