Vincent Tran has posted comments on this change. ( http://gerrit.cloudera.org:8080/7009 )
Change subject: IMPALA-5315: Cast to timestamp fails for YYYY-M-D format ...................................................................... Patch Set 12: (10 comments) http://gerrit.cloudera.org:8080/#/c/7009/12/be/src/exprs/expr-test.cc File be/src/exprs/expr-test.cc: http://gerrit.cloudera.org:8080/#/c/7009/12/be/src/exprs/expr-test.cc@3010 PS12, Line 3010: TimestampValue::Parse("2001-01-21 01:05:1.11211")); > There's some attempt in the code to handle trailing timezone offsets. Can w Removed timezone offset support as it was not part of the request from the Jira. http://gerrit.cloudera.org:8080/#/c/7009/12/be/src/runtime/timestamp-parse-util.h File be/src/runtime/timestamp-parse-util.h: http://gerrit.cloudera.org:8080/#/c/7009/12/be/src/runtime/timestamp-parse-util.h@179 PS12, Line 179: /// Parse a default date/time string. The default timestamp format is: > This comment doesn't seem to match the function. Looks like I inserted my function header between a comment and its function. Fixed http://gerrit.cloudera.org:8080/#/c/7009/12/be/src/runtime/timestamp-parse-util.h@191 PS12, Line 191: static bool ParseFormatTokensByStr(DateTimeFormatContext* dt_ctx); > Nit: add a space before the next function for consistency. Done http://gerrit.cloudera.org:8080/#/c/7009/12/be/src/runtime/timestamp-parse-util.cc File be/src/runtime/timestamp-parse-util.cc: http://gerrit.cloudera.org:8080/#/c/7009/12/be/src/runtime/timestamp-parse-util.cc@218 PS12, Line 218: bool TimestampParser::ParseFormatTokensByStr(DateTimeFormatContext* dt_ctx){ > I'm really struggling to understand this function, particularly in what cas Done. Changed to serial parsing. http://gerrit.cloudera.org:8080/#/c/7009/12/be/src/runtime/timestamp-parse-util.cc@229 PS12, Line 229: if ((*str == 'T') || (*str == 'Z') || (!isalnum(*str))) { > Do we really need to handle arbitrary separators? It seems like this will a No. Changed to only accept '-' for date separator, ':' for time separator and '.' for the fractional second separator. http://gerrit.cloudera.org:8080/#/c/7009/12/be/src/runtime/timestamp-parse-util.cc@232 PS12, Line 232: dt_ctx->toks.push_back(DateTimeFormatToken(TZ_OFFSET, str - str_begin, > Do we need to support timezone offsets here? I guess we don't have to - based on the Jira's description. Removed. http://gerrit.cloudera.org:8080/#/c/7009/12/be/src/runtime/timestamp-parse-util.cc@282 PS12, Line 282: if (tok_len == 1) ++dt_ctx->fmt_out_len; > Do we need to compute fmt_out_len for this case? No. Removed http://gerrit.cloudera.org:8080/#/c/7009/12/be/src/runtime/timestamp-parse-util.cc@292 PS12, Line 292: // lazy_ctx is made local so that we do not run into races where > We can remove this comment (it's interesting but won't have much future val Done http://gerrit.cloudera.org:8080/#/c/7009/12/be/src/runtime/timestamp-parse-util.cc@395 PS12, Line 395: > nit: trailing whitespace Done http://gerrit.cloudera.org:8080/#/c/7009/12/be/src/runtime/timestamp-parse-util.cc@461 PS12, Line 461: date TimestampParser::RealignYear(const DateTimeParseResult& dt_result, > nit: can you move this back to the previous location. It's annoying that th Done -- To view, visit http://gerrit.cloudera.org:8080/7009 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib9a184a09d7e7783f04d47588537612c2ecec28f Gerrit-Change-Number: 7009 Gerrit-PatchSet: 12 Gerrit-Owner: Vincent Tran <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Vincent Tran <[email protected]> Gerrit-Comment-Date: Sat, 10 Feb 2018 23:59:56 +0000 Gerrit-HasComments: Yes
