Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14517 )
Change subject: [KUDU-2632] Add a DATE type backed by INT32 (Part 2, Java client) ...................................................................... Patch Set 2: (19 comments) http://gerrit.cloudera.org:8080/#/c/14517/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/14517/2//COMMIT_MSG@13 PS2, Line 13: LocalDate.parse("0001-01-01").toEpochDay() from http://gerrit.cloudera.org:8080/#/c/14517/2/java/kudu-client/src/main/java/org/apache/kudu/client/KuduPredicate.java File java/kudu-client/src/main/java/org/apache/kudu/client/KuduPredicate.java: http://gerrit.cloudera.org:8080/#/c/14517/2/java/kudu-client/src/main/java/org/apache/kudu/client/KuduPredicate.java@172 PS2, Line 172: trailing space http://gerrit.cloudera.org:8080/#/c/14517/2/java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java File java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java: http://gerrit.cloudera.org:8080/#/c/14517/2/java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java@646 PS2, Line 646: IllegalArgumentException Is it possible with given the Date range constraints? Overall, I think it would be nice to add a unit test for this method to verify the declared behavior. http://gerrit.cloudera.org:8080/#/c/14517/2/java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java@674 PS2, Line 674: a Date nit: Date value stored in the column? http://gerrit.cloudera.org:8080/#/c/14517/2/java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java@692 PS2, Line 692: checkColumn(schema.getColumnByIndex(columnIndex), Type.DATE); : checkColumnExists(schema.getColumnByIndex(columnIndex)); Should these two be swapped? http://gerrit.cloudera.org:8080/#/c/14517/2/java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java@698 PS2, Line 698: nit: trailing spaces http://gerrit.cloudera.org:8080/#/c/14517/2/java/kudu-client/src/main/java/org/apache/kudu/util/DataGenerator.java File java/kudu-client/src/main/java/org/apache/kudu/util/DataGenerator.java: http://gerrit.cloudera.org:8080/#/c/14517/2/java/kudu-client/src/main/java/org/apache/kudu/util/DataGenerator.java@128 PS2, Line 128: date nit: int value representable as DATE ? http://gerrit.cloudera.org:8080/#/c/14517/2/java/kudu-client/src/main/java/org/apache/kudu/util/DateUtil.java File java/kudu-client/src/main/java/org/apache/kudu/util/DateUtil.java: PS2: Add Apache license header. http://gerrit.cloudera.org:8080/#/c/14517/2/java/kudu-client/src/main/java/org/apache/kudu/util/DateUtil.java@9 PS2, Line 9: public class DateUtil { It would be great to add unit tests for the methods of this class. Points of interest for unit tests are boundary conditions (i.e. min/max values and under/over those, etc.) at least. http://gerrit.cloudera.org:8080/#/c/14517/2/java/kudu-client/src/main/java/org/apache/kudu/util/DateUtil.java@10 PS2, Line 10: Trailing spaces here and in few lines in this file. http://gerrit.cloudera.org:8080/#/c/14517/2/java/kudu-client/src/main/java/org/apache/kudu/util/DateUtil.java@11 PS2, Line 11: (int)LocalDate.parse("0001-01-01").toEpochDay(); // -719162 : public static final int MAX_DATE_VALUE = : (int)LocalDate.parse("9999-12-31").toEpochDay(); // 2932896 Could you add information on how these numbers were produced? I understand the principle, but counting number of leap years in a span of years might be a bit non-trivial. http://gerrit.cloudera.org:8080/#/c/14517/2/java/kudu-client/src/main/java/org/apache/kudu/util/DateUtil.java@16 PS2, Line 16: Check nit: Checks The idea is to use the same form of verbs in doc comments throughout the methods. http://gerrit.cloudera.org:8080/#/c/14517/2/java/kudu-client/src/main/java/org/apache/kudu/util/DateUtil.java@22 PS2, Line 22: Date value Does it make sense to provide the value in the error message? http://gerrit.cloudera.org:8080/#/c/14517/2/java/kudu-client/src/main/java/org/apache/kudu/util/DateUtil.java@27 PS2, Line 27: number days number of days http://gerrit.cloudera.org:8080/#/c/14517/2/java/kudu-client/src/main/java/org/apache/kudu/util/DateUtil.java@57 PS2, Line 57: return LocalDate.ofEpochDay(days).toString(); Does it make sense to call checkDateWithinRange() here as it's done in epochDaysToSqlDate() method? http://gerrit.cloudera.org:8080/#/c/14517/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java: http://gerrit.cloudera.org:8080/#/c/14517/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java@653 PS2, Line 653: timestamp date ? http://gerrit.cloudera.org:8080/#/c/14517/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestPartialRow.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestPartialRow.java: http://gerrit.cloudera.org:8080/#/c/14517/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestPartialRow.java@440 PS2, Line 440: addDate Does it make sense to add negative scenarios for PartialRow.addDate() as well (i.e. adding a value not representable as DATE)? http://gerrit.cloudera.org:8080/#/c/14517/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestRowResult.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestRowResult.java: http://gerrit.cloudera.org:8080/#/c/14517/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestRowResult.java@156 PS2, Line 156: nit: trailing spaces http://gerrit.cloudera.org:8080/#/c/14517/2/java/kudu-flume-sink/src/main/java/org/apache/kudu/flume/sink/RegexpKuduOperationsProducer.java File java/kudu-flume-sink/src/main/java/org/apache/kudu/flume/sink/RegexpKuduOperationsProducer.java: http://gerrit.cloudera.org:8080/#/c/14517/2/java/kudu-flume-sink/src/main/java/org/apache/kudu/flume/sink/RegexpKuduOperationsProducer.java@329 PS2, Line 329: trailing spaces -- To view, visit http://gerrit.cloudera.org:8080/14517 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9a09f4f9c804c1b2965e78da78528225a9c769e2 Gerrit-Change-Number: 14517 Gerrit-PatchSet: 2 Gerrit-Owner: Volodymyr Verovkin <verjov...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 13 Nov 2019 18:50:51 +0000 Gerrit-HasComments: Yes