Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/11927 )
Change subject: IMPALA-941: Fix support for an identifier that starts with a number ...................................................................... Patch Set 3: (5 comments) http://gerrit.cloudera.org:8080/#/c/11927/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11927/2//COMMIT_MSG@10 PS2, Line 10: However, its implementation was broken, especially when it : comes to using it in a fully-qualified name > nit: Maybe mention with an example that it was getting lexed to Done http://gerrit.cloudera.org:8080/#/c/11927/2/fe/src/main/jflex/sql-scanner.flex File fe/src/main/jflex/sql-scanner.flex: http://gerrit.cloudera.org:8080/#/c/11927/2/fe/src/main/jflex/sql-scanner.flex@414 PS2, Line 414: a dot followed by an identifier will > I think this stmt is a bit confusing because technically an identifier cann Yeah, it's confusing after re-reading it. Done. http://gerrit.cloudera.org:8080/#/c/11927/2/fe/src/main/jflex/sql-scanner.flex@416 PS2, Line 416: = \`(\\.|[^\\\`])*\` > nit: why extra braces? maybe something like follows? Yeah, not needed. Done. http://gerrit.cloudera.org:8080/#/c/11927/2/fe/src/main/jflex/sql-scanner.flex@511 PS2, Line 511: return newToken(SqlParserSymbols.DOT, yytext()); : } > instead can't we just do No, we need to push back the identifier to the input stream to basically "undo" the read so that the next scan can process the identifier token. If we don't push back we will only get one token (dot) instead of two tokens (dot and identifier). http://gerrit.cloudera.org:8080/#/c/11927/2/fe/src/test/java/org/apache/impala/analysis/ParserTest.java File fe/src/test/java/org/apache/impala/analysis/ParserTest.java: http://gerrit.cloudera.org:8080/#/c/11927/2/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@3903 PS2, Line 3903: bar > for a moment I was wondering why this is a problem :) Maybe mention exponen Done -- To view, visit http://gerrit.cloudera.org:8080/11927 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8a715be73553247ce80a1ba841712191d64f9730 Gerrit-Change-Number: 11927 Gerrit-PatchSet: 3 Gerrit-Owner: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Jim Apple <jbapple-imp...@apache.org> Gerrit-Reviewer: Paul Rogers <par0...@yahoo.com> Gerrit-Comment-Date: Wed, 14 Nov 2018 04:52:51 +0000 Gerrit-HasComments: Yes
