Gabor Kaszab 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 4: (4 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: TestIsNull("months_between('23:33:45','15:33:45')", TYPE_DOUBLE); > I guess you may expect "Epoch time: 1970|01|01 00|00|00" as a result, unix_ Thanks for the explanation! One more question: If we decide not to return Null here, then wouldn't it make sense to return the value as if that "Epoch time: " something wasn't there? I have a feeling that we should either return Null, or return the value indicated by the given parameters, so in this case I think this query should return the same as the following: unix_timestamp("1970|01|01 00|00|00","yyyy|MM|dd HH|mm|ss"); 28800 Could you please share your thoughts? http://gerrit.cloudera.org:8080/#/c/8508/3/be/src/exprs/expr-test.cc@5764 PS3, Line 5764: TestIsNull("dayofweek(cast('09:10:11.000000' as timestamp))", TYPE_INT); > As I explained above, I think returning NULL for the mismatched case is eno Thanks again for the explanation! Using TestIsNull here sounds fine. http://gerrit.cloudera.org:8080/#/c/8508/4/be/src/runtime/timestamp-parse-util.h File be/src/runtime/timestamp-parse-util.h: http://gerrit.cloudera.org:8080/#/c/8508/4/be/src/runtime/timestamp-parse-util.h@182 PS4, Line 182: /// Get string literal between quotes 1: This comment doesn't add any further info as it is basically the function name with spaces. 2: The comment should tell the reader if there is some precondition for this function. In this case I think it would be beneficial to mention that we assume that str is currently pointing to the opening quote. 3: For me it would also make sense to say explicitly what successful and unsuccessful parse mean. e.g. parsing is unsuccessful if there is no closing quote. 4: The 'Get' in the function name indicates that when we call this function it either returns the found string literal as a return value or through an out parameter. I think this is a bit misleading as in fact this function is for 2 different things (but not for returning a string literal): - Finding the closing quote and getting the string between the 2 quotes. - Adding a DateTimeFormatToken to dt_ctx. My preference here would be to introduce functions with a single area of responsibility: one for finding the string between the quotes and one for inserting the FormatToken to dt_ctx. What do you think? http://gerrit.cloudera.org:8080/#/c/8508/4/be/src/runtime/timestamp-parse-util.h@183 PS4, Line 183: /// dt_ctx -- date/time format context Again, this is simply writing the type of the parameter with spaces. -- 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: 4 Gerrit-Owner: Kim Jin Chul <[email protected]> Gerrit-Reviewer: Attila Jeges <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Kim Jin Chul <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Wed, 06 Dec 2017 09:42:15 +0000 Gerrit-HasComments: Yes
