Attila Jeges 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 20: (9 comments) http://gerrit.cloudera.org:8080/#/c/13722/20/be/src/exprs/timestamp-functions-ir.cc File be/src/exprs/timestamp-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/13722/20/be/src/exprs/timestamp-functions-ir.cc@73 PS20, Line 73: buf Maybe this should be called 'buff' to match the formal parameter name in the Format() function. http://gerrit.cloudera.org:8080/#/c/13722/20/be/src/runtime/date-parse-util.cc File be/src/runtime/date-parse-util.cc: http://gerrit.cloudera.org:8080/#/c/13722/20/be/src/runtime/date-parse-util.cc@135 PS20, Line 135: string& buff 'buff' should either be a string* param or the return value of Format() to emphasize that it is an out param/value. http://gerrit.cloudera.org:8080/#/c/13722/20/be/src/runtime/date-parse-util.cc@138 PS20, Line 138: DCHECK(buff.empty()); This probably not necessary. Format should work even if string is not empty on call. http://gerrit.cloudera.org:8080/#/c/13722/20/be/src/runtime/date-parse-util.cc@180 PS20, Line 180: buff.append(string(str_val, str_val_len)); No need to create a temp string, you can append char* directly: buff.append(str_val, str_val_len) http://gerrit.cloudera.org:8080/#/c/13722/20/be/src/runtime/date-parse-util.cc@180 PS20, Line 180: buff.append(string(str_val, str_val_len)); I'm not sure what happens if str_val == nullptr and str_val_len == 0. Maybe we should check for that before calling append() just to be safe? http://gerrit.cloudera.org:8080/#/c/13722/20/be/src/runtime/date-value.cc File be/src/runtime/date-value.cc: http://gerrit.cloudera.org:8080/#/c/13722/20/be/src/runtime/date-value.cc@86 PS20, Line 86: string& buff Again, 'buff' should be a string* param or the return value of Format(). http://gerrit.cloudera.org:8080/#/c/13722/20/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/13722/20/be/src/runtime/datetime-iso-sql-format-tokenizer.cc@113 PS20, Line 113: DCHECK(curr_token_size > 0); nit: Probably not necessary, L110 and L111 implies that his is true. http://gerrit.cloudera.org:8080/#/c/13722/20/be/src/runtime/timestamp-parse-util.cc File be/src/runtime/timestamp-parse-util.cc: http://gerrit.cloudera.org:8080/#/c/13722/20/be/src/runtime/timestamp-parse-util.cc@218 PS20, Line 218: void TimestampParser::Format(const DateTimeFormatContext& dt_ctx, : const date& d, const time_duration& t, string& buff) { : DCHECK(dt_ctx.toks.size() > 0); : DCHECK(buff.length() == 0); : if (dt_ctx.has_date_toks && d.is_special()) return; : if (dt_ctx.has_time_toks && t.is_special()) return; : for (const DateTimeFormatToken& tok: dt_ctx.toks) { : int32_t num_val = -1; : const char* str_val = NULL; : int str_val_len = 0; : switch (tok.type) { : case YEAR: : case ROUND_YEAR: { : num_val = d.year(); : if (tok.len < 4) { : int adjust_factor = std::pow(10, tok.len); : num_val %= adjust_factor; : } : break; : } : case MONTH_IN_YEAR: num_val = d.month().as_number(); break; : case MONTH_IN_YEAR_SLT: { : str_val = d.month().as_short_string(); : str_val_len = 3; : break; : } : case DAY_IN_MONTH: num_val = d.day(); break; : case DAY_IN_YEAR: { : num_val = GetDayInYear(d.year(), d.month(), d.day()); : break; : } : case HOUR_IN_DAY: num_val = t.hours(); break; : case HOUR_IN_HALF_DAY: { : num_val = t.hours(); : if (num_val == 0) num_val = 12; : if (num_val > 12) num_val -= 12; : break; : } : case MERIDIEM_INDICATOR: { : const MERIDIEM_INDICATOR_TEXT* indicator_txt = (tok.len == 2) ? &AM : &AM_LONG; : if (t.hours() >= 12) { : indicator_txt = (tok.len == 2) ? &PM : &PM_LONG; : } : str_val_len = tok.len; : str_val = (isupper(*tok.val)) ? indicator_txt->first : indicator_txt->second; : break; : } : case MINUTE_IN_HOUR: num_val = t.minutes(); break; : case SECOND_IN_MINUTE: num_val = t.seconds(); break; : case SECOND_IN_DAY: { : num_val = t.hours() * 3600 + t.minutes() * 60 + t.seconds(); : break; : } : case FRACTION: { : num_val = t.fractional_seconds(); : if (num_val > 0) for (int j = tok.len; j < 9; ++j) num_val /= 10; : break; : } : case SEPARATOR: : case ISO8601_TIME_INDICATOR: : case ISO8601_ZULU_INDICATOR: { : str_val = tok.val; : str_val_len = tok.len; : break; : } : case TZ_OFFSET: { : break; : } : default: DCHECK(false) << "Unknown date/time format token"; : } : if (num_val > -1) { : string tmp_str = std::to_string(num_val); : if (tmp_str.length() < tok.len) tmp_str.insert(0, tok.len - tmp_str.length(), '0'); : buff.append(tmp_str); : } else { : buff.append(string(str_val, str_val_len)); : } : } Same as comments on DateParser::Format(). http://gerrit.cloudera.org:8080/#/c/13722/20/be/src/runtime/timestamp-value.cc File be/src/runtime/timestamp-value.cc: http://gerrit.cloudera.org:8080/#/c/13722/20/be/src/runtime/timestamp-value.cc@82 PS20, Line 82: string& buff Again, buff should be a string* param or the return value of Format() to emphasize that it is on out param/value. -- 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: 20 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: Tue, 17 Sep 2019 13:53:10 +0000 Gerrit-HasComments: Yes
