Lars Volker has posted comments on this change. Change subject: IMPALA-3381: Support AM/PM marker in date and time format strings ......................................................................
Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/6523/1/be/src/exprs/expr-test.cc File be/src/exprs/expr-test.cc: Line 5641: // AM/PM marker can be repeated and placed anywhere in the format string Do we have a check that prevents having multiple separate am/pm markers? http://gerrit.cloudera.org:8080/#/c/6523/1/be/src/runtime/timestamp-parse-util.cc File be/src/runtime/timestamp-parse-util.cc: Line 173: case 'a': tok_type = AM_PM_MARKER; dt_ctx->has_am_pm_marker = true; break; Have you considered supporting 'am' and 'pm' as tokens, too, like Greg suggested in the JIRA? Line 201: if (tok_len != 2) { You could remove this if statement (and add 0 below). PS1, Line 467: strncmp You can use strncasecmp() and simplify the code. -- To view, visit http://gerrit.cloudera.org:8080/6523 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I99794a3e152f1712c6c469bb266d23a81d19ca34 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Attila Jeges <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-HasComments: Yes
