Gabor Kaszab has posted comments on this change. ( http://gerrit.cloudera.org:8080/13722 )
Change subject: IMPALA-8703: ISO:SQL:2016 datetime patterns - Milestone 1 ...................................................................... Patch Set 4: (12 comments) Attila, with patch set 4 I have covered all your findings. The perf degradation is still an issue, though. http://gerrit.cloudera.org:8080/#/c/13722/2/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/13722/2/be/src/runtime/datetime-iso-sql-format-parser.cc@184 PS2, Line 184: &result->day)) { : return false; : } : } : : return true; : } : : const char* DateTimeIsoSqlFormatParser::GetNextTokenGroupFromInput(const char* input_str, : int input_len, const DateTimeFormatToken& tok) { : DCHECK(input_str != nullptr); : : // Handle separately the meridiem indicators for two reasons. : // 1: They might contain '.' that is not meant to be a separator character. : // 2: The length of the token in the pattern might differ from the length of the token : // in the input. E.g. "AM" should match with "P.M.". : if (tok.type == MERIDIEM_IND > This can be done more efficiently if you use hard-coded month ranges, like Done http://gerrit.cloudera.org:8080/#/c/13722/2/be/src/runtime/datetime-iso-sql-format-parser.cc@184 PS2, Line 184: &result->day)) { : return false; : } : } : : return true; : } : : const char* DateTimeIsoSqlFormatParser::GetNextTokenGroupFromInput(const char* input_str, : int input_len, const DateTimeFormatToken& tok) { : DCHECK(input_str != nullptr); : : // Handle separately the meridiem indicators for two reasons. : // 1: They might contain '.' that is not meant to be a separator character. : // 2: The length of the token in the pattern might differ from the length of the token : // in the input. E.g. "AM" should match with "P.M.". : if (tok.type == MERIDIEM_IND > Move this to a separate function. Done http://gerrit.cloudera.org:8080/#/c/13722/3/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/13722/3/be/src/runtime/datetime-iso-sql-format-parser.cc@78 PS3, Line 78: if (!ParseAndValidate(current_pos, group_len, 0, 9999, &result->year)) { > line too long (92 > 90) Done http://gerrit.cloudera.org:8080/#/c/13722/3/be/src/runtime/datetime-iso-sql-format-parser.cc@95 PS3, Line 95: } > line too long (91 > 90) Done http://gerrit.cloudera.org:8080/#/c/13722/3/be/src/runtime/datetime-iso-sql-format-parser.cc@124 PS3, Line 124: if (!ParseAndValidate(current_pos, group_len, 0, 23, &result->hour)) return false; > line too long (92 > 90) Done http://gerrit.cloudera.org:8080/#/c/13722/3/be/src/runtime/datetime-iso-sql-format-parser.cc@128 PS3, Line 128: if (!ParseAndValidate(current_pos, group_len, 0, 59, &result->minute)) { > line too long (92 > 90) Done http://gerrit.cloudera.org:8080/#/c/13722/3/be/src/runtime/datetime-iso-sql-format-parser.cc@133 PS3, Line 133: case SECOND_IN_MINUTE: { > line too long (94 > 90) Done http://gerrit.cloudera.org:8080/#/c/13722/2/be/src/runtime/datetime-parser-common.cc File be/src/runtime/datetime-parser-common.cc: http://gerrit.cloudera.org:8080/#/c/13722/2/be/src/runtime/datetime-parser-common.cc@185 PS2, Line 185: : : : : : : : : : : > This function can be simplified if you use hard-coded month ranges. (See MO Done http://gerrit.cloudera.org:8080/#/c/13722/3/be/src/runtime/datetime-simple-date-format-parser.h File be/src/runtime/datetime-simple-date-format-parser.h: http://gerrit.cloudera.org:8080/#/c/13722/3/be/src/runtime/datetime-simple-date-format-parser.h@98 PS3, Line 98: /// accept_time_toks_only -- if true, time tokens w/o date tokens are accepted. > line too long (92 > 90) Done http://gerrit.cloudera.org:8080/#/c/13722/3/be/src/runtime/datetime-simple-date-format-parser.h@104 PS3, Line 104: /// Parse date/time string to find the corresponding default date/time format context. > line too long (92 > 90) Done http://gerrit.cloudera.org:8080/#/c/13722/3/be/src/runtime/datetime-simple-date-format-parser.h@160 PS3, Line 160: /// Does only a basic validation on the parsed date/time values. The caller is > line too long (92 > 90) Done http://gerrit.cloudera.org:8080/#/c/13722/2/be/src/runtime/datetime-simple-date-format-parser.h File be/src/runtime/datetime-simple-date-format-parser.h: http://gerrit.cloudera.org:8080/#/c/13722/2/be/src/runtime/datetime-simple-date-format-parser.h@68 PS2, Line 68: /// Long literal months e.g. MMMM > Maybe the stuff in this file could be moved to separate classes, like Simpl Done -- To view, visit http://gerrit.cloudera.org:8080/13722 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I19d8d097a45ae6f103b6cd1b2d81aad38dfd9e23 Gerrit-Change-Number: 13722 Gerrit-PatchSet: 4 Gerrit-Owner: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Attila Jeges <atti...@cloudera.com> Gerrit-Reviewer: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Comment-Date: Wed, 10 Jul 2019 09:37:25 +0000 Gerrit-HasComments: Yes
