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 4: (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 without trimming 'buff'. http://gerrit.cloudera.org:8080/#/c/14714/4/be/src/runtime/datetime-parser-common.cc@225 PS4, Line 225: const string& actual_suffix = : buff.substr(SHORT_MONTH_NAME_LENGTH, expected_suffix.length()); : if (actual_suffix != expected_suffix) return false; : *month = it->second.second; It would be faster to use a const char* pointer and strncmp() for comparison instead of creating an 'actual_suffix' string instance. http://gerrit.cloudera.org:8080/#/c/14714/4/be/src/runtime/datetime-parser-common.cc@232 PS4, Line 232: const string month_name = prefix + actual_suffix; No need to create a new string instance. Only the length of 'month_name' is used in L233-236. -- 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: 4 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 10:25:19 +0000 Gerrit-HasComments: Yes
