Dan Burkert has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10502 )

Change subject: [java] Add Timestamp APIs to kudu-client
......................................................................


Patch Set 1:

(10 comments)

http://gerrit.cloudera.org:8080/#/c/10502/1/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/10502/1/java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java@557
PS1, Line 557:   public void addTimestamp(int columnIndex, Timestamp val) {
Document how java.sql.Timestamp instances with nanosecond precision are 
truncated or otherwise made to be written to Kudu's timestamp type, which is 
microsecond precise.


http://gerrit.cloudera.org:8080/#/c/10502/1/java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java@566
PS1, Line 566:    * Add a Timestamp for the specified column.
ditto


http://gerrit.cloudera.org:8080/#/c/10502/1/java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java@586
PS1, Line 586:   public Timestamp getTimestamp(String columnName) {
Probably worth documenting that all returned values will be in the UTC timezone 
offset (+0), which may be confusing since values can be written with any 
timezone offset.


http://gerrit.cloudera.org:8080/#/c/10502/1/java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java@593
PS1, Line 593:    * @param columnIndex Column index in the schema
ditto


http://gerrit.cloudera.org:8080/#/c/10502/1/java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java
File java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java:

http://gerrit.cloudera.org:8080/#/c/10502/1/java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java@354
PS1, Line 354:    * @return a BigDecimal
Update


http://gerrit.cloudera.org:8080/#/c/10502/1/java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java@366
PS1, Line 366:    * @return a BigDecimal
likewise


http://gerrit.cloudera.org:8080/#/c/10502/1/java/kudu-client/src/main/java/org/apache/kudu/util/TimestampUtil.java
File java/kudu-client/src/main/java/org/apache/kudu/util/TimestampUtil.java:

http://gerrit.cloudera.org:8080/#/c/10502/1/java/kudu-client/src/main/java/org/apache/kudu/util/TimestampUtil.java@42
PS1, Line 42:    * Converts a [[Timestamp]] to microseconds since the Unix 
epoch (1970-01-01T00:00:00Z).
I think the brackets are scaladoc specific.  Javadoc equivalent is @link or 
something like that


http://gerrit.cloudera.org:8080/#/c/10502/1/java/kudu-client/src/main/java/org/apache/kudu/util/TimestampUtil.java@60
PS1, Line 60: [[Timestamp]
likewise


http://gerrit.cloudera.org:8080/#/c/10502/1/java/kudu-client/src/main/java/org/apache/kudu/util/TimestampUtil.java@83
PS1, Line 83:   public static String timestampToString(long micros) {
Shouldn't be necessary to have both versions of this helper anymore, since the 
caller of the other version could use the alternate getter.


http://gerrit.cloudera.org:8080/#/c/10502/1/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/10502/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestPartialRow.java@36
PS1, Line 36: public class TestPartialRow {
Add a tests that writes a timestamp with a non-zulu offset and ensure it's 
properly handled.



--
To view, visit http://gerrit.cloudera.org:8080/10502
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1fdd397691f12f279663838af9fa12ed27508fd1
Gerrit-Change-Number: 10502
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke <[email protected]>
Gerrit-Reviewer: Adar Dembo <[email protected]>
Gerrit-Reviewer: Dan Burkert <[email protected]>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <[email protected]>
Gerrit-Comment-Date: Wed, 30 May 2018 21:35:47 +0000
Gerrit-HasComments: Yes

Reply via email to