Attila Jeges has posted comments on this change. ( http://gerrit.cloudera.org:8080/14714 )
Change subject: IMPALA-8705: ISO:SQL:2016 datetime patterns - Milestone 3 ...................................................................... Patch Set 5: (10 comments) http://gerrit.cloudera.org:8080/#/c/14714/5/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/14714/5/be/src/runtime/datetime-iso-sql-format-parser.cc@297 PS5, Line 297: if (tok.type == MONTH_NAME && fx_provided && !tok.fm_modifier) { : if (input_len < MAX_MONTH_NAME_LENGTH) return nullptr; : return input_str + MAX_MONTH_NAME_LENGTH; : } What happens if tok.type == MONTH_NAME && (!fx_provided || tok.fm_modifier)? I think in that case, FindEndOfToken() doesn't return the proper end of the token, since month names are not necessarily followed by a separator. http://gerrit.cloudera.org:8080/#/c/14714/5/be/src/runtime/datetime-parser-common.h File be/src/runtime/datetime-parser-common.h: http://gerrit.cloudera.org:8080/#/c/14714/5/be/src/runtime/datetime-parser-common.h@43 PS5, Line 43: const int MONTH_LENGTHS[12] = {31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31}; Is this still used? http://gerrit.cloudera.org:8080/#/c/14714/5/be/src/runtime/datetime-parser-common.h@57 PS5, Line 57: number nit: ordinal number http://gerrit.cloudera.org:8080/#/c/14714/5/be/src/runtime/datetime-parser-common.h@63 PS5, Line 63: number nit: ordinal number http://gerrit.cloudera.org:8080/#/c/14714/5/be/src/runtime/datetime-parser-common.h@70 PS5, Line 70: /// Mapping between textual month name and the number of month. Used for string to : /// datetime conversions. : const std::unordered_map<s Fix comment. http://gerrit.cloudera.org:8080/#/c/14714/5/be/src/runtime/datetime-parser-common.h@88 PS5, Line 88: 'FEB closing ' is missing. http://gerrit.cloudera.org:8080/#/c/14714/5/be/src/runtime/datetime-parser-common.h@295 PS5, Line 295: /// If the month part of the input is not followed by a separator then the end of the : /// month part is found by probing the input with different lengths. Please fix the comment. http://gerrit.cloudera.org:8080/#/c/14714/5/be/src/runtime/datetime-parser-common.h@308 PS5, Line 308: MONTH_LENGTHS MONTH_RANGES and LEAP_YEAR_MONTH_RANGES http://gerrit.cloudera.org:8080/#/c/14714/5/be/src/runtime/datetime-parser-common.h@328 PS5, Line 328: one nit: 1 http://gerrit.cloudera.org:8080/#/c/14714/5/be/src/runtime/datetime-parser-common.cc File be/src/runtime/datetime-parser-common.cc: http://gerrit.cloudera.org:8080/#/c/14714/5/be/src/runtime/datetime-parser-common.cc@193 PS5, Line 193: const char* input_end This parametr is not really needed. It is used only in a DCHECK. -- To view, visit http://gerrit.cloudera.org:8080/14714 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic797f19a1311b54e5d00d01d0a7afe1f0f21fb8f Gerrit-Change-Number: 14714 Gerrit-PatchSet: 5 Gerrit-Owner: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Attila Jeges <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Comment-Date: Wed, 27 Nov 2019 16:11:48 +0000 Gerrit-HasComments: Yes
