Attila Jeges has posted comments on this change. ( http://gerrit.cloudera.org:8080/14291 )
Change subject: IMPALA-8704: ISO:SQL:2016 datetime patterns - Milestone 2 ...................................................................... Patch Set 7: (7 comments) http://gerrit.cloudera.org:8080/#/c/14291/7/be/src/runtime/datetime-iso-sql-format-parser.h File be/src/runtime/datetime-iso-sql-format-parser.h: http://gerrit.cloudera.org:8080/#/c/14291/7/be/src/runtime/datetime-iso-sql-format-parser.h@78 PS7, Line 78: format text token inside the format http://gerrit.cloudera.org:8080/#/c/14291/7/be/src/runtime/datetime-iso-sql-format-parser.h@81 PS7, Line 81: format text token http://gerrit.cloudera.org:8080/#/c/14291/7/be/src/runtime/datetime-iso-sql-format-parser.h@81 PS7, Line 81: and moves : // '*format' to 'a'. I don't think part this is true. In GetNextCharFromTextToken() *format is moved to the last character of the escape sequence (to '"' in the example given above). http://gerrit.cloudera.org:8080/#/c/14291/7/be/src/runtime/datetime-iso-sql-format-parser.cc File be/src/runtime/datetime-iso-sql-format-parser.cc: http://gerrit.cloudera.org:8080/#/c/14291/7/be/src/runtime/datetime-iso-sql-format-parser.cc@48 PS7, Line 48: if (tok->type == SEPARATOR && !dt_ctx.fx_modifier) { : bool res = ProcessSeparatorSequence(¤t_pos, end_pos, dt_ctx, &i); : if (!res || current_pos == end_pos) return res; : DCHECK(i < dt_ctx.toks.size()); : // Next token, following the separator sequence. : tok = &dt_ctx.toks[i]; : } I'd prefer to handle strict separator matching (tok->type == SEPARATOR && dt_ctx.fx_modifier) right after this, instead of L183. http://gerrit.cloudera.org:8080/#/c/14291/7/be/src/runtime/datetime-iso-sql-format-parser.cc@56 PS7, Line 56: const char* token_end_pos = FindEndOfToken(current_pos, end_pos - current_pos, *tok); : if (token_end_pos == nullptr) return false; : int token_len = token_end_pos - current_pos; : : if (dt_ctx.fx_modifier && !tok->fm_modifier && tok->type != TEXT && : token_len != tok->len) { : return false; : } for TEXT tokens, 'token_end_pos' and 'token_len' aren't set here. 'token_end_pos' is set in L170. For clarity I'd move the code in L170-182 right before L56, and execute L56-193 only if tok->type != TEXT. http://gerrit.cloudera.org:8080/#/c/14291/7/be/src/runtime/datetime-iso-sql-format-parser.cc@185 PS7, Line 185: token_len == 1 dt_ctx.fx_modifier && token_len == 1 http://gerrit.cloudera.org:8080/#/c/14291/7/be/src/runtime/datetime-parser-common.h File be/src/runtime/datetime-parser-common.h: http://gerrit.cloudera.org:8080/#/c/14291/7/be/src/runtime/datetime-parser-common.h@134 PS7, Line 134: then that -- To view, visit http://gerrit.cloudera.org:8080/14291 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I30d2f6656054371476aaa8bd0d51f572b9369855 Gerrit-Change-Number: 14291 Gerrit-PatchSet: 7 Gerrit-Owner: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Attila Jeges <atti...@cloudera.com> Gerrit-Reviewer: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Comment-Date: Wed, 16 Oct 2019 12:11:14 +0000 Gerrit-HasComments: Yes
