Shirish Gajera has posted comments on this change. Change subject: IMPALA-2033: Netezza compatibility functions quote_ident ......................................................................
Patch Set 1: (5 comments) I'll rename quote_ident to netezza_quote_ident. I already submitted ICLA yesterday. http://gerrit.cloudera.org:8080/#/c/3263/1/be/src/exprs/expr-test.cc File be/src/exprs/expr-test.cc: Line 1958: TestStringValue("quote_ident('100')", "\"100\""); > Can you test something starting with a number and ending with alpha. Will add this test case in next patch. http://gerrit.cloudera.org:8080/#/c/3263/1/be/src/exprs/string-functions-ir.cc File be/src/exprs/string-functions-ir.cc: Line 761: std::string all_lowercase(reinterpret_cast<const char*>(str.ptr), str.len); > You should be able to use string without the std:: prefix. The "common/name will remove std in next patch. Line 767: const std::string numeric("01234567890_"); > This is really non_alpha, right? yes, numeric means non_alpha, but i'll rename it to non_alpha. Line 768: if (all_lowercase.find_first_not_of(lowercase_numeric) == std::string::npos) { > Maybe use && instead of nested if statements? will do it in next patch. Line 769: if (all_lowercase.find_first_not_of(numeric) != std::string::npos) { > Shouldn't this be checking for whether the first character is numeric? this is for the case when the input is only number. e:g quote_ident("100") => "100" (inside loop will return false) e:g quote_ident("abc100") => abc100 (inside loop will return true) -- To view, visit http://gerrit.cloudera.org:8080/3263 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Idbac4cd10fe08a6bd4871a03ba3b5f2c20fa8ee1 Gerrit-PatchSet: 1 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Shirish Gajera <[email protected]> Gerrit-Reviewer: Shirish Gajera <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
