Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/13648 )
Change subject: IMPALA-7369: part 2: Add INTERVAL expr support and built-in functions for DATE ...................................................................... Patch Set 1: (7 comments) The change seems ok to me in general, congrats for the great test coverage. http://gerrit.cloudera.org:8080/#/c/13648/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13648/1//COMMIT_MSG@105 PS1, Line 105: 1 This should be 0 according to the link. http://gerrit.cloudera.org:8080/#/c/13648/1/be/src/exprs/expr-test.cc File be/src/exprs/expr-test.cc: http://gerrit.cloudera.org:8080/#/c/13648/1/be/src/exprs/expr-test.cc@7777 PS1, Line 7777: // 2019-01-01 is Tuesday, it belongs to the first wek of the year. typo: week http://gerrit.cloudera.org:8080/#/c/13648/1/be/src/exprs/expr-test.cc@7778 PS1, Line 7778: TestValue("weekofyear(date '2018-12-31')", TYPE_INT, 1); The comment for WeekOfYear() states that it can return values 1-53. Can you add a test where it returns 53? For example select weekofyear("2015-12-31"); returns 53 in Hive. http://gerrit.cloudera.org:8080/#/c/13648/1/be/src/runtime/date-test.cc File be/src/runtime/date-test.cc: http://gerrit.cloudera.org:8080/#/c/13648/1/be/src/runtime/date-test.cc@845 PS1, Line 845: // 2019-12-30 (Monday) and 2019-12-31 (Tuesday) belong to year 2020. Can you add a test with week number 53? See expr-test.cc for details. http://gerrit.cloudera.org:8080/#/c/13648/1/be/src/runtime/date-test.cc@910 PS1, Line 910: EXPECT_TRUE(DateValue(2016, 2, 29).MonthsBetween( : DateValue(2016, 1, 29), &months_between)); : EXPECT_EQ(1, months_between); optional: a helper function like TestMonthsBetween(DateValue dt1, DateValue dt2, int min_expected, int max_expected) could make this function less verbose http://gerrit.cloudera.org:8080/#/c/13648/1/be/src/runtime/date-test.cc@936 PS1, Line 936: monrhs_between typo: months_between http://gerrit.cloudera.org:8080/#/c/13648/1/be/src/runtime/date-value.h File be/src/runtime/date-value.h: http://gerrit.cloudera.org:8080/#/c/13648/1/be/src/runtime/date-value.h@121 PS1, Line 121: AddMonths I would prefer to move this to .cc / inline.h 'keep_last_day' could be also an argument, I don't think that we really benefit from it being a template. -- To view, visit http://gerrit.cloudera.org:8080/13648 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If404bffdaf055c769e79ffa8f193bac415cfdd1a Gerrit-Change-Number: 13648 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-Comment-Date: Fri, 14 Jun 2019 15:50:08 +0000 Gerrit-HasComments: Yes
