Gabor Kaszab has posted comments on this change. ( http://gerrit.cloudera.org:8080/14291 )
Change subject: IMPALA-8704: ISO:SQL:2016 datetime patterns - Milestone 2 ...................................................................... Patch Set 12: (7 comments) http://gerrit.cloudera.org:8080/#/c/14291/11/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/14291/11/be/src/runtime/datetime-iso-sql-format-parser.cc@214 PS11, Line 214: DCHECK(tok->val <= *format && *format < tok->val + tok->len); > DCHECK(tok->val <= *format < tok->val + tok->len); Done http://gerrit.cloudera.org:8080/#/c/14291/11/be/src/runtime/datetime-iso-sql-format-parser.cc@217 PS11, Line 217: ped quo > Is it possible that strncmp (here or in the next line) overruns [tok->val, Indeed, the text token part is not null-terminated. This wouldn't cause any issues in my opinion es the format as a whole is null-terminated. Adding an extra check to be on the safe side. http://gerrit.cloudera.org:8080/#/c/14291/11/be/src/runtime/datetime-iso-sql-format-parser.cc@255 PS11, Line 255: if (*current_pos >= end_pos && *current_tok_idx < dt_ctx.toks.size()) { > Add *current_tok_idx < dt_ctx.toks.size() condition to the while condition Thx, done. http://gerrit.cloudera.org:8080/#/c/14291/11/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/14291/11/be/src/runtime/datetime-iso-sql-format-tokenizer.cc@84 PS11, Line 84: const char* str_end = dt_ctx_->fmt + dt_ctx_->fmt_len; > As explained in L271, 'str_end' is not needed. See answer in L271. http://gerrit.cloudera.org:8080/#/c/14291/11/be/src/runtime/datetime-iso-sql-format-tokenizer.cc@111 PS11, Line 111: const char* str_end = dt_ctx_->fmt + dt_ctx_->fmt_len; > As explained in L271, 'str_end' is not needed. See answer in L271 http://gerrit.cloudera.org:8080/#/c/14291/11/be/src/runtime/datetime-iso-sql-format-tokenizer.cc@271 PS11, Line 271: while (current_pos < str_end) { > If we assume that 'str_start' points to a null-terminated string, we don't Yeah, technically I could drop 'str_end' and compare 'current_pos' to '\0'. However, I feel that having 'str_end' improves code readability a bit. http://gerrit.cloudera.org:8080/#/c/14291/11/tests/query_test/test_cast_with_format.py File tests/query_test/test_cast_with_format.py: http://gerrit.cloudera.org:8080/#/c/14291/11/tests/query_test/test_cast_with_format.py@1101 PS11, Line 1101: FXFMFXYYYY-MM-DD > 'FXFXYYYY-MM-DD' should fail too right? Done -- To view, visit http://gerrit.cloudera.org:8080/14291 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I30d2f6656054371476aaa8bd0d51f572b9369855 Gerrit-Change-Number: 14291 Gerrit-PatchSet: 12 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: Tue, 29 Oct 2019 17:39:15 +0000 Gerrit-HasComments: Yes
