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 4: (12 comments) http://gerrit.cloudera.org:8080/#/c/14714/3/be/src/runtime/date-parse-util.cc File be/src/runtime/date-parse-util.cc: http://gerrit.cloudera.org:8080/#/c/14714/3/be/src/runtime/date-parse-util.cc@160 PS3, Line 160: case MONTH_NAME_SHORT: { : result.append(FormatMonthName(month, tok)); : break; : } > Instead of "DDD" I meant "MMM" above of course :) Nice catch. I haven't run a full test suite on the latest patch. Done. http://gerrit.cloudera.org:8080/#/c/14714/3/be/src/runtime/datetime-iso-sql-format-tokenizer.cc File be/src/runtime/datetime-iso-sql-format-tokenizer.cc: http://gerrit.cloudera.org:8080/#/c/14714/3/be/src/runtime/datetime-iso-sql-format-tokenizer.cc@200 PS3, Line 200: t provided_month_tokens = IsUsedToken("MM") + IsUsedToken("MONTH") + : IsUsedToken("MON"); > Probably it would be simpler to do this check with a counter, similarly to Done http://gerrit.cloudera.org:8080/#/c/14714/3/be/src/runtime/datetime-iso-sql-format-tokenizer.cc@207 PS3, Line 207: sUsedToken("MM") || IsUsedToken("MONTH > You probably have to add: Done http://gerrit.cloudera.org:8080/#/c/14714/3/be/src/runtime/datetime-parser-common.h File be/src/runtime/datetime-parser-common.h: http://gerrit.cloudera.org:8080/#/c/14714/3/be/src/runtime/datetime-parser-common.h@22 PS3, Line 22: #include <unordered_map> : #include <unordered_set> : #include <vector> > nit: order includes alphabetically. Done http://gerrit.cloudera.org:8080/#/c/14714/3/be/src/runtime/datetime-parser-common.h@70 PS3, Line 70: /// Mapping between textual month name and > I think this constant and the others below should be int instead. Done http://gerrit.cloudera.org:8080/#/c/14714/3/be/src/runtime/datetime-parser-common.h@75 PS3, Line 75: > Closing apostrophe is missing. Done http://gerrit.cloudera.org:8080/#/c/14714/3/be/src/runtime/datetime-parser-common.h@273 PS3, Line 273: > MONTH_NAME_SHORT Done http://gerrit.cloudera.org:8080/#/c/14714/3/be/src/runtime/datetime-parser-common.h@315 PS3, Line 315: r, int days_ > nit: month or day name Done http://gerrit.cloudera.org:8080/#/c/14714/3/be/src/runtime/datetime-parser-common.cc File be/src/runtime/datetime-parser-common.cc: http://gerrit.cloudera.org:8080/#/c/14714/3/be/src/runtime/datetime-parser-common.cc@199 PS3, Line 199: token_len = * > (token_len < SHORT_MONTH_NAME_LENGTH) Done http://gerrit.cloudera.org:8080/#/c/14714/3/be/src/runtime/datetime-parser-common.cc@223 PS3, Line 223: const string& expected_suffix = it->second.first; : if (buff.length() - SHORT_MONTH_NAME_LENGTH < expected_suffix.length()) return false; : const string& actual_suffix = : buff.substr(SHORT_MONTH_NAME_LENGTH, expected_suffix.length()); : if (actual_suffix != expected_suffix) return false; : *month = it->second.second; : : // If the end of the month token wasn't identified because it wasn't followed by a : // separator then the end of the month token has to be adjusted. : const string month_name = prefix + actual_suffix; : if (month_name.length() < buff.length()) { : > I think, we could do this faster by taking advantage of the fact that check Done http://gerrit.cloudera.org:8080/#/c/14714/3/be/src/runtime/datetime-parser-common.cc@329 PS3, Line 329: DCHECK(tok.type == DAY_NAME || tok.type == DAY_NAME_SHORT || tok.type == MONTH_NAME || > Add: && tok.len >= SHORT_MONTH_NAME_LENGTH condition. Done http://gerrit.cloudera.org:8080/#/c/14714/3/be/src/runtime/timestamp-parse-util.cc File be/src/runtime/timestamp-parse-util.cc: http://gerrit.cloudera.org:8080/#/c/14714/3/be/src/runtime/timestamp-parse-util.cc@255 PS3, Line 255: case DAY_OF_WEEK: { > Please add a comment explaining that 1 means Sunday, 2 means Monday, and so 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: 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: Tue, 26 Nov 2019 09:03:39 +0000 Gerrit-HasComments: Yes
