You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2019/11/13 18:50:51 UTC
[kudu-CR] [KUDU-2632] Add a DATE type backed by INT32 (Part 2, Java client)
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 <ve...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 13 Nov 2019 18:50:51 +0000
Gerrit-HasComments: Yes