Tim Armstrong has posted comments on this change. Change subject: IMPALA-2033: Netezza compatibility functions quote_ident ......................................................................
Patch Set 1: (5 comments) Thanks for the patch! We had a conversation a while ago about how/whether to implement quote_ident. My comment back then was: I think we need to think carefully about quote_ident. The problem is that Impala's rules for identifiers, e.g. `ident` are different from Netezza's, e.g. "an identifier with spaces" - Impala only supports alphanumeric characters and underscores (no spaces) and identifiers are quoted by backticks rather than double quotes. I think we should consider renaming it to something like netezza_quote_ident to avoid confusion. So I think we should rename this netezza_quote_ident to avoid confusion. Also, have you looked into submitting a CLA: https://github.com/cloudera/Impala/wiki/Contributing-to-Impala? 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. E.g. "100abcd" and "_test" 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:: You should be able to use string without the std:: prefix. The "common/names.h" include takes care of that. Line 767: numeric This is really non_alpha, right? Line 768: if (all_lowercase.find_first_not_of(lowercase_numeric) == std::string::npos) { Maybe use && instead of nested if statements? 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? -- 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: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
