Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8508 )
Change subject: IMPALA-5237: Support a quoted string in date/time format ...................................................................... Patch Set 9: (5 comments) Thank you for the fix. Just had a final round of comments to make it a bit easier to understand. http://gerrit.cloudera.org:8080/#/c/8508/9/be/src/exprs/expr-test.cc File be/src/exprs/expr-test.cc: http://gerrit.cloudera.org:8080/#/c/8508/9/be/src/exprs/expr-test.cc@6631 PS9, Line 6631: TestStringValue(R"(from_unixtime(0, 'H\'foo\'\'bar\'H'))", "0foo'bar0"); Can you add a comment, e.g. // Test that pairs of single quotes are treated as an escape sequence. http://gerrit.cloudera.org:8080/#/c/8508/9/be/src/exprs/expr-test.cc@6632 PS9, Line 6632: TestStringValue(R"(from_unixtime(0, 'H\'foo\'H\'bar\'H'))", "0foo0bar0"); Can you also test the case when the escaped quote is at the end of the string, e.g. 'foobar''' http://gerrit.cloudera.org:8080/#/c/8508/9/be/src/runtime/timestamp-parse-util.cc File be/src/runtime/timestamp-parse-util.cc: http://gerrit.cloudera.org:8080/#/c/8508/9/be/src/runtime/timestamp-parse-util.cc@86 PS9, Line 86: nit: remove extra line http://gerrit.cloudera.org:8080/#/c/8508/9/be/src/runtime/timestamp-parse-util.cc@175 PS9, Line 175: // quote due to the compatibility with Hive's behavior. This is a little tricky and needs some explanation. I'd suggest something like: "We treat the first of the pair of quotes as the last character of the current string literal and continue parsing from the second quote, treating it as the start of a new string literal. This avoids the need to copy the string literal." http://gerrit.cloudera.org:8080/#/c/8508/9/be/src/runtime/timestamp-parse-util.cc@177 PS9, Line 177: +1 nit: spaces around +, i.e. should be " + 1" -- To view, visit http://gerrit.cloudera.org:8080/8508 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie34055ac695748bcfb110bfa6ed5308f469ea178 Gerrit-Change-Number: 8508 Gerrit-PatchSet: 9 Gerrit-Owner: Kim Jin Chul <[email protected]> Gerrit-Reviewer: Attila Jeges <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Kim Jin Chul <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Tue, 20 Feb 2018 23:32:44 +0000 Gerrit-HasComments: Yes
