Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12689 )
Change subject: [java] Add private diff scan support ...................................................................... Patch Set 8: (7 comments) http://gerrit.cloudera.org:8080/#/c/12689/8/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/12689/8/java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java@a251 PS8, Line 251: > Is there any concern that someone was previously abusing getLong() by using I don't think so. Doing so would return the indirectData offset which isn't documented anywhere and is an internal detail. Additionally getting the indirectData offset is not useful to them because they can't use it to get a value from any public methods. The only thing this would expose are bugs and data loss/corruption. http://gerrit.cloudera.org:8080/#/c/12689/8/java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java@253 PS8, Line 253: return Bytes.getLong(this.rowData.getRawArray(), : this.rowData.getRawOffset() + : getCurrentRowDataOffsetForColumn(columnIndex)); > This can chain to getIndirectDataOffset. Even though the "code" is the same I don't call that function since this is actually getting the long value and not the indirectData offset. http://gerrit.cloudera.org:8080/#/c/12689/8/java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java@370 PS8, Line 370: long micros = Bytes.getLong(this.rowData.getRawArray(), : this.rowData.getRawOffset() + : getCurrentRowDataOffsetForColumn(columnIndex)); > As can this. See above. http://gerrit.cloudera.org:8080/#/c/12689/8/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/12689/8/java/kudu-client/src/main/java/org/apache/kudu/util/TimestampUtil.java@97 PS8, Line 97: public static String timestampToString(long micros) { > Kinda confusing that this method claims to convert a "timestamp" into a str This is a hold over from when the only way to get a timestamp was via getLong on a UNIXTIME_MICROS column. In that context I think this makes sense. http://gerrit.cloudera.org:8080/#/c/12689/8/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java: http://gerrit.cloudera.org:8080/#/c/12689/8/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@266 PS8, Line 266: it's > Nit: its Done http://gerrit.cloudera.org:8080/#/c/12689/8/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@269 PS8, Line 269: Map<Integer, ChangeType> results = new HashMap<>(); > Worth noting that, except for logging, the test currently doesn't use these The return value is stored in the `mutations` variable and used to lookup and compare against the scanned results. http://gerrit.cloudera.org:8080/#/c/12689/8/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@336 PS8, Line 336: operations > operation Done -- To view, visit http://gerrit.cloudera.org:8080/12689 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I51b01ae76cd22407df9097b56629ac7262ec2964 Gerrit-Change-Number: 12689 Gerrit-PatchSet: 8 Gerrit-Owner: Grant Henke <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Greg Solovyev <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Comment-Date: Mon, 18 Mar 2019 17:44:21 +0000 Gerrit-HasComments: Yes
