[Impala-ASF-CR] IMPALA-5315: Cast to timestamp fails for YYYY-M-D format

["Vincent Tran (Code Review)" <ge...@cloudera.org>]

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 <vt...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vincent Tran <vt...@cloudera.com>
Gerrit-Comment-Date: Sat, 10 Feb 2018 23:59:56 +0000
Gerrit-HasComments: Yes