Zoltan Chovan has posted comments on this change. ( http://gerrit.cloudera.org:8080/23482 )
Change subject: KUDU-1261 [Java] Add write support for Array Type ...................................................................... Patch Set 11: (11 comments) http://gerrit.cloudera.org:8080/#/c/23482/11/java/kudu-client/src/main/java/org/apache/kudu/client/Array1dSerdes.java File java/kudu-client/src/main/java/org/apache/kudu/client/Array1dSerdes.java: http://gerrit.cloudera.org:8080/#/c/23482/11/java/kudu-client/src/main/java/org/apache/kudu/client/Array1dSerdes.java@26 PS11, Line 26: import com.google.flatbuffers.Table; it seems like this import is never used, remove? http://gerrit.cloudera.org:8080/#/c/23482/11/java/kudu-client/src/main/java/org/apache/kudu/client/ArrayCellView.java File java/kudu-client/src/main/java/org/apache/kudu/client/ArrayCellView.java: http://gerrit.cloudera.org:8080/#/c/23482/11/java/kudu-client/src/main/java/org/apache/kudu/client/ArrayCellView.java@73 PS11, Line 73: "Index " + i + " out of bounds for validity length " + len) nit: change this to String.format, just for consistency across this class http://gerrit.cloudera.org:8080/#/c/23482/11/java/kudu-client/src/main/java/org/apache/kudu/client/ArrayCellView.java@125 PS11, Line 125: getUInt16 seems like these getUIntX() methods are still not used and were not removed as commented previously http://gerrit.cloudera.org:8080/#/c/23482/11/java/kudu-client/src/main/java/org/apache/kudu/client/ArrayCellView.java@217 PS11, Line 217: valuesAsByteBuffer could this return null? http://gerrit.cloudera.org:8080/#/c/23482/11/java/kudu-client/src/main/java/org/apache/kudu/client/ArrayCellView.java@261 PS11, Line 261: (byte) what is the point of this cast? http://gerrit.cloudera.org:8080/#/c/23482/11/java/kudu-client/src/main/java/org/apache/kudu/client/ArrayCellView.java@267 PS11, Line 267: i -> arr.values(i) here and some of the other case branches, the lambda could be replaced by a method reference (arr::values) http://gerrit.cloudera.org:8080/#/c/23482/11/java/kudu-client/src/main/java/org/apache/kudu/client/ArrayCellView.java@273 PS11, Line 273: (short) what is the point of this cast? http://gerrit.cloudera.org:8080/#/c/23482/11/java/kudu-client/src/main/java/org/apache/kudu/client/ArrayCellView.java@329 PS11, Line 329: valuesLength could this return null? http://gerrit.cloudera.org:8080/#/c/23482/11/java/kudu-client/src/main/java/org/apache/kudu/client/ArrayCellView.java@357 PS11, Line 357: <A> this parameter is never used http://gerrit.cloudera.org:8080/#/c/23482/11/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/23482/11/java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java@2007 PS11, Line 2007: toString() is this toString() necessary? http://gerrit.cloudera.org:8080/#/c/23482/11/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/23482/11/java/kudu-client/src/test/java/org/apache/kudu/client/TestPartialRow.java@292 PS11, Line 292: asList here and at other places in this class: Arrays.asList() could be replaced with Collections.SingletonList() since there is only one element added to the list, and the SingletonList will be immutable. not really a dealbreaker, just a small nit, you can leave this as is, since other already existing tests use the same style -- To view, visit http://gerrit.cloudera.org:8080/23482 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icbe5e243eafe12a8977d40204dacab99624451eb Gerrit-Change-Number: 23482 Gerrit-PatchSet: 11 Gerrit-Owner: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Comment-Date: Thu, 09 Oct 2025 09:30:59 +0000 Gerrit-HasComments: Yes
