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 6: (12 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) If there is no following separator then the month name token will have the length of MAX_MONTH_NAME_LENGTH and later on it will be adjusted in ParseMonthNameToken(). See L233-236 in datetime-parser-common.cc I added a comment to the header about this. 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 FRACTIONAL_SECOND_MAX_LENGTH = 9; > Is this still used? Removed http://gerrit.cloudera.org:8080/#/c/14714/5/be/src/runtime/datetime-parser-common.h@57 PS5, Line 57: ir<std > nit: ordinal number Done http://gerrit.cloudera.org:8080/#/c/14714/5/be/src/runtime/datetime-parser-common.h@63 PS5, Line 63: > nit: ordinal number Done http://gerrit.cloudera.org:8080/#/c/14714/5/be/src/runtime/datetime-parser-common.h@59 PS5, Line 59: {"jan", {"uary", 1}}, : {"feb", {"ruary", 2}}, : {"mar", {"ch", 3}}, : {"apr", {"il", 4}}, : {"may", {"", 5}}, : {"jun", {"e", 6}}, : {"jul", {"y", 7}}, : {"aug", {"ust", 8}}, : {"sep", {"tember", 9}}, : {"oct", {"ober These are no longer needed since the introduction of MONTH_PREFIX_TO_SUFFIX. Dropped http://gerrit.cloudera.org:8080/#/c/14714/5/be/src/runtime/datetime-parser-common.h@70 PS5, Line 70: {"dec", {"ember", 12}} : }; : > Fix comment. Being a bit more specific here would help. I don't think that this comment is broken. This is a mapping between textual month name and the ordinal number of month. So this part is true. This is used for string to datetime conversion so this part is true as well. I could mention in the comment that the key part of the map is the 3-letter prefix of the month names and this is mapped to the rest of the month name and the number of month but anyone who takes a look at this part of the code would see this immediately without reading the comment so I didn't see the point. http://gerrit.cloudera.org:8080/#/c/14714/5/be/src/runtime/datetime-parser-common.h@88 PS5, Line 88: > closing ' is missing. Done http://gerrit.cloudera.org:8080/#/c/14714/5/be/src/runtime/datetime-parser-common.h@295 PS5, Line 295: : /// Gets a year and the number of days passed since 1st of January t > Please fix the comment. Done http://gerrit.cloudera.org:8080/#/c/14714/5/be/src/runtime/datetime-parser-common.h@308 PS5, Line 308: casing of the > MONTH_RANGES and LEAP_YEAR_MONTH_RANGES Instead I removed the end of the comment. http://gerrit.cloudera.org:8080/#/c/14714/5/be/src/runtime/datetime-parser-common.h@328 PS5, Line 328: Wee > nit: 1 Done 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: int* month) { > This parametr is not really needed. It is used only in a DCHECK. Done http://gerrit.cloudera.org:8080/#/c/14714/5/be/src/runtime/datetime-parser-common.cc@235 PS5, Line 235: > Maybe this would be more straightforward: Agree. 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: 6 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: Thu, 28 Nov 2019 11:21:44 +0000 Gerrit-HasComments: Yes
