Attila Jeges 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: (6 comments) 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 over Done 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. Done 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 Done 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? I Done 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 functio Added some tests to DateValueEdgeCases and ParseFormatEdgeCases 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 Timesta Done -- 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: Wed, 26 Sep 2018 10:57:03 +0000 Gerrit-HasComments: Yes
