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 2: (6 comments) http://gerrit.cloudera.org:8080/#/c/8818/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/8818/2//COMMIT_MSG@7 PS2, Line 7: wronly wrongly http://gerrit.cloudera.org:8080/#/c/8818/2//COMMIT_MSG@10 PS2, Line 10: There are some holes in escaping the string literal Can you expand on this a little more, eg mention that: - toSql() always returns strings that are single quoted, resulting in improper escaping in the output if the original string was actually double quoted - Its not always possible to determine if a string "should" be single or double quoted, eg concat('a', "b") and that's why we just choose one to always normalize to. http://gerrit.cloudera.org:8080/#/c/8818/2/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/2/fe/src/main/java/org/apache/impala/analysis/StringLiteral.java@39 PS2, Line 39: private final String cached_escaped_value_; Add a comment explaining what this is. Also, per my comments below, let's get rid of the concept of this being a 'cached' value, and name it something like 'escaped_single_quoted_value_'. http://gerrit.cloudera.org:8080/#/c/8818/2/fe/src/main/java/org/apache/impala/analysis/StringLiteral.java@104 PS2, Line 104: getEscapedValue > Escaping and unescaping the value happen here. Do you have more intuitive n You might name it something like "getEscapedSingleQuotedValue" to make it more clear what its doing. http://gerrit.cloudera.org:8080/#/c/8818/2/fe/src/main/java/org/apache/impala/analysis/StringLiteral.java@105 PS2, Line 105: if (cached_escaped_value_ != null) return cached_escaped_value_; Since getEscapedValue() is called in the constructor, this will always be true outside the constructor. So, let's simplify the logic here by removing this and only calling getEscapedValue() in the constructor, then using the cached_escaped_value_ directly in the other places, eg. toSqlImpl(). http://gerrit.cloudera.org:8080/#/c/8818/2/fe/src/main/java/org/apache/impala/analysis/StringLiteral.java@121 PS2, Line 121: i++ nit: ++i -- 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: 2 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: Sat, 06 Jan 2018 00:44:30 +0000 Gerrit-HasComments: Yes
