Kim Jin Chul has posted comments on this change. ( http://gerrit.cloudera.org:8080/8508 )
Change subject: IMPALA-5237: Support a quoted string in date/time format ...................................................................... Patch Set 3: (6 comments) http://gerrit.cloudera.org:8080/#/c/8508/3/be/src/exprs/expr-test.cc File be/src/exprs/expr-test.cc: http://gerrit.cloudera.org:8080/#/c/8508/3/be/src/exprs/expr-test.cc@5746 PS3, Line 5746: TestValue("unix_timestamp('\\'Epoch time: \\'1970|01|01 00|00|00',\ > I checked and Hive in fact returns NULL for this input. I guess you may expect "Epoch time: 1970|01|01 00|00|00" as a result, unix_timestamp is not suitable because it returns bigint. I think two kinds of results are only available: zero or NULL As you mentioned, Hive returns NULL because the quoted string might be recognized as an unexpected format string. The string literal in the quotes is meaningless in unix_timestamp function, but I think an user wants to add some description and it should not affect returning a result. The flexibility might be helpful to an user even though a difference happens between Impala and Hive. Therefore, the result should be zero in Impala. http://gerrit.cloudera.org:8080/#/c/8508/3/be/src/exprs/expr-test.cc@5756 PS3, Line 5756: // 2) Not allowd unclosed quoted string > nit: typo: allowed Done http://gerrit.cloudera.org:8080/#/c/8508/3/be/src/exprs/expr-test.cc@5764 PS3, Line 5764: TestValue("unix_timestamp('\\'Epoch time: \\'1970|01|01 00|00|00',\ > There is one more thing I don't understand: Regardless of giving this forma As I explained above, I think returning NULL for the mismatched case is enough. I would like to prefer NULL instead of throwing an error. There are two issues. 1) The expected result is NULL because the input does not fit to the format(e.g. Epoch and Enoch). I think returning NULL is enough for happening a mismatch internally. 2) I should have to use TestIsNull instead of TestValue. It's my mistake when I copied from the above. Let me fix this. By the way, there is a different problem in our test framework. I guess the following test should fail: TestValue(cast(null as int), TYPE_INT, 0) As you know, NULL cannot be compared with any value because it is unknown, so it should not be accept. I filed the issue on IMPALA-6283. http://gerrit.cloudera.org:8080/#/c/8508/3/be/src/runtime/timestamp-parse-util.h File be/src/runtime/timestamp-parse-util.h: http://gerrit.cloudera.org:8080/#/c/8508/3/be/src/runtime/timestamp-parse-util.h@77 PS3, Line 77: /// IMPALA-5237: Support a quoted string in date/time format > I have the feeling that copying the whole commit message here is a bit of a Done http://gerrit.cloudera.org:8080/#/c/8508/3/be/src/runtime/timestamp-parse-util.cc File be/src/runtime/timestamp-parse-util.cc: http://gerrit.cloudera.org:8080/#/c/8508/3/be/src/runtime/timestamp-parse-util.cc@163 PS3, Line 163: if (*str == '\'') { > I would find this part of code a bit more self-descriptive if it was in the Done http://gerrit.cloudera.org:8080/#/c/8508/3/be/src/runtime/timestamp-parse-util.cc@173 PS3, Line 173: if (len == 0) len = 1; > The naming of len, val and pos aren't that self-explanatory for me. Would i Done -- To view, visit http://gerrit.cloudera.org:8080/8508 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie34055ac695748bcfb110bfa6ed5308f469ea178 Gerrit-Change-Number: 8508 Gerrit-PatchSet: 3 Gerrit-Owner: Kim Jin Chul <jinc...@gmail.com> Gerrit-Reviewer: Attila Jeges <atti...@cloudera.com> Gerrit-Reviewer: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Kim Jin Chul <jinc...@gmail.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Wed, 06 Dec 2017 07:56:51 +0000 Gerrit-HasComments: Yes
