Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/12009 )
Change subject: IMPALA-7905: Hive keywords not quoted for identifiers ...................................................................... Patch Set 6: (6 comments) http://gerrit.cloudera.org:8080/#/c/12009/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12009/1//COMMIT_MSG@17 PS1, Line 17: insensitive i > typo Done http://gerrit.cloudera.org:8080/#/c/12009/5/fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java File fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java: http://gerrit.cloudera.org:8080/#/c/12009/5/fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java@116 PS5, Line 116: an identifier > nit: 'ident' "ident" is a common abbreviation, and the name of the parameter, but I think the English term is "identifier." Do we normally use abbreviations in comments? http://gerrit.cloudera.org:8080/#/c/12009/5/fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java@124 PS5, Line 124: upper-case text, but that > probably switch to ANTLRNoCaseStringStream to be consistent with Hive? Turns out ANTLRNoCaseStringStream is a non-static inner class inside the ParserDriver class for no good reason other than the author seems to have forgotten to type "static". All that ANTLRNoCaseStringStream does is uppercase each character as it is returned. Uppercasing the entire (short) string up front accomplishes the same goal here, so should be OK. http://gerrit.cloudera.org:8080/#/c/12009/5/fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java@148 PS5, Line 148: / : public static boolean impalaNeedsQuotes(String ident) { : return SqlScanner.isReserved(ident) || : // Impala's scanner recognizes the ".123" portion of "db.123e45" as a decimal, : // so while the quoting is not necessary > Actually, I think IMPALA-941 recently fixed this bad decimal thing. Made the || change. Retained this code from lines 131-136 of the original code. Fredy tells me that although his change can handle ".123" as an identifier, it cannot handle ".123e3". So, I'm hesitant to remove this code. http://gerrit.cloudera.org:8080/#/c/12009/5/fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java@169 PS5, Line 169: / > Why this special case? (optimization?) Since * is not a valid identifier, it would normally be quoted. But, here is is the wildcard character which should not be quoted. So, yes it does need special handling. Added a comment. http://gerrit.cloudera.org:8080/#/c/12009/5/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java File fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java: http://gerrit.cloudera.org:8080/#/c/12009/5/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java@186 PS5, Line 186: String quotedCol = ToSqlUtils.identSql(col); > Can we do col = ToSqlUtils.identSql(col) here instead? Done -- To view, visit http://gerrit.cloudera.org:8080/12009 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I06cc20b052a3a66535a171c36b4b31477c0ba6d0 Gerrit-Change-Number: 12009 Gerrit-PatchSet: 6 Gerrit-Owner: Paul Rogers <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Paul Rogers <[email protected]> Gerrit-Comment-Date: Thu, 27 Dec 2018 07:56:36 +0000 Gerrit-HasComments: Yes
