Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/11450 )
Change subject: IMPALA-7492: Add support for DATE text parser/formatter ...................................................................... Patch Set 1: (5 comments) I have went through the change quickly, I will dig into details deeper in the future. http://gerrit.cloudera.org:8080/#/c/11450/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11450/2//COMMIT_MSG@13 PS2, Line 13: The parser supports parsing both default and custom formatted DATE : values. The formatter supports default and custom formatting of DATE : values. It took me some time to realize that these two sentences are not duplicates. Please add "also" or merge them somehow. http://gerrit.cloudera.org:8080/#/c/11450/2/be/src/runtime/date-test.cc File be/src/runtime/date-test.cc: http://gerrit.cloudera.org:8080/#/c/11450/2/be/src/runtime/date-test.cc@195 PS2, Line 195: TEST I would prefer to split this into more than one tests, e.g. DefaultParse, CustomParse, EdgeCases, Format. timestamp-test.cc has a very long Basic test, but I do not think that it is a good idea - I found it quite hard to get an overview of the tests and checking whether something is already tested or not. http://gerrit.cloudera.org:8080/#/c/11450/2/be/src/runtime/date-test.cc@491 PS2, Line 491: const int64_t MIN_DATE_DAYS_SINCE_EPOCH = -719528; Does this have to be int64? Int32 should be generally always enough to represent day since epoch. http://gerrit.cloudera.org:8080/#/c/11450/2/be/src/runtime/date-test.cc@510 PS2, Line 510: const int64_t MAX_DATE_DAYS_SINCE_EPOCH = 2932896; Same as line 491. http://gerrit.cloudera.org:8080/#/c/11450/2/be/src/runtime/date-value.h File be/src/runtime/date-value.h: http://gerrit.cloudera.org:8080/#/c/11450/2/be/src/runtime/date-value.h@35 PS2, Line 35: 9999-01-01 Isn't it 9999-12-31? That date can be represented with TimestampValue. -- To view, visit http://gerrit.cloudera.org:8080/11450 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1eec00f22502c4c67c6807c4b51384419ea8b831 Gerrit-Change-Number: 11450 Gerrit-PatchSet: 1 Gerrit-Owner: Attila Jeges <atti...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Mon, 17 Sep 2018 16:19:59 +0000 Gerrit-HasComments: Yes
