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 17: (7 comments) 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. Done http://gerrit.cloudera.org:8080/#/c/13722/17/be/src/exprs/cast-format-expr.h@37 PS17, Line 37: virtual > nit: same Done http://gerrit.cloudera.org:8080/#/c/13722/17/be/src/exprs/cast-format-expr.h@46 PS17, Line 46: FunctionContext > const Done 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* Done 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); There are tokens where the input they accept is actually longer than the length of the token. e.g. MONTH, DAY, DY, FF[4-9] AM, PM. So limiting the input string length not to be longer than the max length of the format string won't be correct. 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); Same as above. 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. On the callsite I prevent calling this function when current_pos == str_end so I modified the DCHECK in this line. -- 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 15:37:18 +0000 Gerrit-HasComments: Yes
