Tianyi Wang has posted comments on this change. Change subject: IMPALA-3894: Change the behavior parsing date "YY" ......................................................................
Patch Set 8: (7 comments) http://gerrit.cloudera.org:8080/#/c/7530/4/be/src/exprs/timestamp-functions-ir.cc File be/src/exprs/timestamp-functions-ir.cc: Line 159: if (!context->IsArgConstant(1)) { > We should be able to do this during the Prepare() phase of this function. I Done http://gerrit.cloudera.org:8080/#/c/7530/4/be/src/runtime/timestamp-parse-util.cc File be/src/runtime/timestamp-parse-util.cc: Line 449: if (tok_len <= 2) realign_year = true; > You can do this on one line (see https://google.github.io/styleguide/cppgui Done PS4, Line 549: r parsed be YY, th > Let's factor this common subexpression out into a separate variable for rea Done Line 551: date parsed_date(dt_result->year, dt_result->month, dt_result->day); > Could you add a comment giving an example of what the value might be at thi Done Line 555: // For example if the century start at 1937 but dt_result->year = 1936, > Could you add a comment giving an example of when we need to increment by 1 Done http://gerrit.cloudera.org:8080/#/c/7530/4/be/src/runtime/timestamp-parse-util.h File be/src/runtime/timestamp-parse-util.h: Line 139: TimestampValue now; > nit: add a "." for consistency with other comments. Done Line 140: > Let's just store a copy of the TimestampValue inline instead of the pointer Done -- To view, visit http://gerrit.cloudera.org:8080/7530 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5da761255915dc741f1dcc488fd4ef6ecc385896 Gerrit-PatchSet: 8 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tianyi Wang <[email protected]> Gerrit-Reviewer: Greg Rahn <[email protected]> Gerrit-Reviewer: Tianyi Wang <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
