Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/8818 )
Change subject: IMPALA-3942: Fix wronly escaped string literal in front-end ...................................................................... Patch Set 6: (6 comments) Code looks good to me, thanks for the contribution! I have just one more round of comments, all about keeping your comments clear and concise. After that, we should be able to pull a +2-er in http://gerrit.cloudera.org:8080/#/c/8818/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/8818/6//COMMIT_MSG@7 PS6, Line 7: wronly wrongly http://gerrit.cloudera.org:8080/#/c/8818/6//COMMIT_MSG@15 PS6, Line 15: and that's why we just choose one : to always normalize to remove this, covered by the paragraph below. http://gerrit.cloudera.org:8080/#/c/8818/6//COMMIT_MSG@19 PS6, Line 19: . ... to single quotes http://gerrit.cloudera.org:8080/#/c/8818/6/fe/src/main/java/org/apache/impala/analysis/StringLiteral.java File fe/src/main/java/org/apache/impala/analysis/StringLiteral.java: http://gerrit.cloudera.org:8080/#/c/8818/6/fe/src/main/java/org/apache/impala/analysis/StringLiteral.java@41 PS6, Line 41: It is not always possible to determine if a string "should" be : // single or double quoted(e.g. concat('a', "b")) and that's why we just choose one : // to always normalize to. This second sentence is not necessary, this is explained better in the larger comment below anyways. http://gerrit.cloudera.org:8080/#/c/8818/6/fe/src/main/java/org/apache/impala/analysis/StringLiteral.java@99 PS6, Line 99: . : // Here is a background why we should deploy single quotes as a default Its nice to keep comments short, so this can just be reduced to 'because:' http://gerrit.cloudera.org:8080/#/c/8818/6/fe/src/main/java/org/apache/impala/analysis/StringLiteral.java@103 PS6, Line 103: It is easy to know if a given string is wrapped by single/double quotes, but Similarly, in the interest of being concise, this can be removed. -- To view, visit http://gerrit.cloudera.org:8080/8818 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibc4b5f5d8ffaa8feb96a466959427a04b3b06fec Gerrit-Change-Number: 8818 Gerrit-PatchSet: 6 Gerrit-Owner: Kim Jin Chul <jinc...@gmail.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Jim Apple <jbapple-imp...@apache.org> Gerrit-Reviewer: Kim Jin Chul <jinc...@gmail.com> Gerrit-Reviewer: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Thu, 11 Jan 2018 21:14:02 +0000 Gerrit-HasComments: Yes
