Alex Behm 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: (5 comments) 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@44 PS6, Line 44: private final String escaped_single_quoted_value_; Let's not store this value and instead compute it on-the-fly. http://gerrit.cloudera.org:8080/#/c/8818/6/fe/src/main/java/org/apache/impala/analysis/StringLiteral.java@110 PS6, Line 110: private String getEscapedSingleQuotedValue() { getNormalizedValue() http://gerrit.cloudera.org:8080/#/c/8818/6/fe/src/main/java/org/apache/impala/analysis/StringLiteral.java@211 PS6, Line 211: + escaped_single_quoted_value_ + "' to number."); let's keep value_ here because that was the original value the user passed http://gerrit.cloudera.org:8080/#/c/8818/6/tests/query_test/test_queries.py File tests/query_test/test_queries.py: http://gerrit.cloudera.org:8080/#/c/8818/6/tests/query_test/test_queries.py@273 PS6, Line 273: result = self.execute_query("select 'a\"b'") My understanding is that these tests do not cover your new changes. These are just plain-old string literals and we return their StringLiteral.value_ Still it's nice to have these StringLiteral tests! They belong in expr-test.cc though http://gerrit.cloudera.org:8080/#/c/8818/6/tests/query_test/test_queries.py@310 PS6, Line 310: def test_escaping_string_literal(self, unique_database): We should have a more focused test for this in ToSqlTest.java. This fix affects StringLiteral.toSql() -- 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: Fri, 12 Jan 2018 23:59:27 +0000 Gerrit-HasComments: Yes
