Gabor Kaszab 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: (3 comments) http://gerrit.cloudera.org:8080/#/c/14714/4/be/src/runtime/datetime-parser-common.cc File be/src/runtime/datetime-parser-common.cc: http://gerrit.cloudera.org:8080/#/c/14714/4/be/src/runtime/datetime-parser-common.cc@212 PS4, Line 212: if (fx_modifier && !tok.fm_modifier) { : DCHECK(buff.length() == MAX_MONTH_NAME_LENGTH); : trim_right_if(buff, is_any_of(" ")); : } > This block is probably not necessary. The parsing below should succeed with If I remove this block then L233-234 would fail for "padded" month names (like "May "). Additionaly L233-234 is needed not to accept "2010-JANUARY-10" with "FXYYYY-MONTH-DD" where the month name is not padded to 9 length. http://gerrit.cloudera.org:8080/#/c/14714/4/be/src/runtime/datetime-parser-common.cc@225 PS4, Line 225: const char* actual_suffix = buff.c_str() + SHORT_MONTH_NAME_LENGTH; : if (strncmp(actual_suffix, expected_suffix.c_str(), expected_suffix.length()) != 0) { : return false; : } > It would be faster to use a const char* pointer and strncmp() for comparis Done http://gerrit.cloudera.org:8080/#/c/14714/4/be/src/runtime/datetime-parser-common.cc@232 PS4, Line 232: // separator then the end of the month token has > No need to create a new string instance. Only the length of 'month_name' is Done -- 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 12:02:31 +0000 Gerrit-HasComments: Yes
