Kim Jin Chul 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 10: (5 comments) Thanks for your comments. I've revised the algorithm to find escaped single quotes. I think this is more intuitive and readable than previous version. 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: // Test that pairs of single quotes are treated as an escape sequence. > Can you add a comment, e.g. Done 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\'\'bar\'H'))", "0foo'bar0"); > Can you also test the case when the escaped quote is at the end of the stri Done 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 Done http://gerrit.cloudera.org:8080/#/c/8508/9/be/src/runtime/timestamp-parse-util.cc@175 PS9, Line 175: // e.g. '''foo''bar''' => 'foo'bar' > This is a little tricky and needs some explanation. I'd suggest something l Let me provide a more intuitive algorithm to find escaped single quotes between single quotes. http://gerrit.cloudera.org:8080/#/c/8508/9/be/src/runtime/timestamp-parse-util.cc@177 PS9, Line 177: q > nit: spaces around +, i.e. should be " + 1" Done -- 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: 10 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: Thu, 22 Feb 2018 02:31:21 +0000 Gerrit-HasComments: Yes
