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 17: (7 comments) Couple more comments. I can +2 it once these are done. http://gerrit.cloudera.org:8080/#/c/13722/17/be/src/exprs/cast-format-expr.h File be/src/exprs/cast-format-expr.h: http://gerrit.cloudera.org:8080/#/c/13722/17/be/src/exprs/cast-format-expr.h@35 PS17, Line 35: virtual nit: 'virtual' keyword is not necessary. 'override' keyword already indicates that this is an override of a virtual function in the bases class. http://gerrit.cloudera.org:8080/#/c/13722/17/be/src/exprs/cast-format-expr.h@37 PS17, Line 37: virtual nit: same http://gerrit.cloudera.org:8080/#/c/13722/17/be/src/exprs/cast-format-expr.h@46 PS17, Line 46: FunctionContext const http://gerrit.cloudera.org:8080/#/c/13722/17/be/src/exprs/cast-format-expr.cc File be/src/exprs/cast-format-expr.cc: http://gerrit.cloudera.org:8080/#/c/13722/17/be/src/exprs/cast-format-expr.cc@64 PS17, Line 64: FunctionContext const FunctionContext* http://gerrit.cloudera.org:8080/#/c/13722/17/be/src/exprs/cast-functions-ir.cc File be/src/exprs/cast-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/13722/17/be/src/exprs/cast-functions-ir.cc@181 PS17, Line 181: int buf_len = format_ctx->fmt_out_len + 1; DCHECK(buf_len <= IsoSqlFormatTokenizer::MAX_FORMAT_LENGTH); http://gerrit.cloudera.org:8080/#/c/13722/17/be/src/exprs/cast-functions-ir.cc@203 PS17, Line 203: int buf_len = format_ctx->fmt_out_len + 1; DCHECK(buf_len <= IsoSqlFormatTokenizer::MAX_FORMAT_LENGTH); http://gerrit.cloudera.org:8080/#/c/13722/17/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/17/be/src/runtime/datetime-iso-sql-format-tokenizer.cc@110 PS17, Line 110: DCHECK(*current_pos <= str_end); Please revisit this DCHECK and the one in L113. If you use '<=' here, you should use '>=' in L113. -- 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: 17 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: Thu, 05 Sep 2019 08:37:10 +0000 Gerrit-HasComments: Yes