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 3: Code-Review+1 (6 comments) I have some minor comments, lgtm otherwise. http://gerrit.cloudera.org:8080/#/c/11450/3/be/src/runtime/date-test.cc File be/src/runtime/date-test.cc: http://gerrit.cloudera.org:8080/#/c/11450/3/be/src/runtime/date-test.cc@50 PS3, Line 50: char s1[] = "2012-01-20"; : char s2[] = "1990-10-20"; : char s3[] = " 1990-10-20 "; : : DateValue v1 = DateValue::Parse(s1, strlen(s1)); : DateValue v2 = DateValue::Parse(s2, strlen(s2)); : DateValue v3 = DateValue::Parse(s3, strlen(s3)); : : ValidateDate(v1, 2012, 1, 20, s1); : ValidateDate(v2, 1990, 10, 20, s2); : ValidateDate(v3, 1990, 10, 20, s3); It would be nice to merge these 3 line tests to 1, e.g. by creating an overload for validate that expects const char* instead of DateValue. http://gerrit.cloudera.org:8080/#/c/11450/3/be/src/runtime/date-test.cc@126 PS3, Line 126: const char* str; This could be renamed to month_name to make its purpose clearer. http://gerrit.cloudera.org:8080/#/c/11450/3/be/src/runtime/date-test.cc@300 PS3, Line 300: char buff[buff_len]; I would prefer to avoid using variable length arrays as it is not standard C++. http://gerrit.cloudera.org:8080/#/c/11450/3/be/src/runtime/date-test.cc@356 PS3, Line 356: const TimestampValue now(date(1980, 2, 28), time_duration(16, 14, 24)); Can you add some tests that parse y/yy with a different "now", e.g. 2018? It does not has to be exhaustive, it should just check that "now" actually matters. http://gerrit.cloudera.org:8080/#/c/11450/3/be/src/runtime/date-test.cc@366 PS3, Line 366: (DateTC("yy-MM-dd", "00-02-29", false, true)) If I didn't miss something, then leap years are only tested in this function. Can you add some checks to the basic tests about 02-29's handling in leap/non-leap years? http://gerrit.cloudera.org:8080/#/c/11450/3/be/src/runtime/date-value.h File be/src/runtime/date-value.h: http://gerrit.cloudera.org:8080/#/c/11450/3/be/src/runtime/date-value.h@59 PS3, Line 59: DateValue(int32_t days_since_epoch) : days_since_epoch_(days_since_epoch) { : } I am not totally sure here, but I would consider a similar logic to TimestampValue, where out of range values are always replaced with a fix invalid value (INVALID_DAYS_SINCE_EPOCH in this case). The benefit would be that IsValid() could be probably faster (test for equality with one value vs two comparisons). Note that this was more important for TimestampValue, because out of range boost dates could lead to exceptions. -- 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: 3 Gerrit-Owner: Attila Jeges <atti...@cloudera.com> Gerrit-Reviewer: Andrew Sherman <asher...@cloudera.com> Gerrit-Reviewer: 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, 24 Sep 2018 14:17:57 +0000 Gerrit-HasComments: Yes
