Abhishek Chennaka 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 10: (13 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? Done http://gerrit.cloudera.org:8080/#/c/23482/7/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/7/java/kudu-client/src/main/java/org/apache/kudu/client/ArrayCellView.java@93 PS7, Line 93: public byte getInt8(int i) { : ensureTag(ScalarArray.Int8Array, "INT8"); : if (!isValid(i)) { : throw new IllegalStateException("Element " + i + " is NULL"); : } : Int8Array arr = new Int8Array(); : content.data(arr); : return arr.values(i); : } > In addition to these per-element accessors, does it make sense to provide a Yes, this will be done in the other patch which implements the read support http://gerrit.cloudera.org:8080/#/c/23482/10/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/10/java/kudu-client/src/main/java/org/apache/kudu/client/ArrayCellView.java@352 PS10, Line 352: appendArrayValues > nit: could this be strAppendArrayValues() or appendArrayValuesToString() or Done 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: w new IndexOutOfBoundsException(); > nit: change this to String.format, just for consistency across this class Done 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 Ah, I thought I got rid of them. Okay, removed them now. 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? Yes, It now throws an meaningful error. http://gerrit.cloudera.org:8080/#/c/23482/11/java/kudu-client/src/main/java/org/apache/kudu/client/ArrayCellView.java@261 PS11, Line 261: rr.val > what is the point of this cast? Redundant, removed it http://gerrit.cloudera.org:8080/#/c/23482/11/java/kudu-client/src/main/java/org/apache/kudu/client/ArrayCellView.java@267 PS11, Line 267: r.values(i) & 0xFF) > here and some of the other case branches, the lambda could be replaced by a Done http://gerrit.cloudera.org:8080/#/c/23482/11/java/kudu-client/src/main/java/org/apache/kudu/client/ArrayCellView.java@273 PS11, Line 273: arr.val > what is the point of this cast? Redundant, removed it 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? No, it can only return an Integer. http://gerrit.cloudera.org:8080/#/c/23482/11/java/kudu-client/src/main/java/org/apache/kudu/client/ArrayCellView.java@357 PS11, Line 357: .ap > this parameter is never used Good catch. Removed it. 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: > is this toString() necessary? Done 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 w I think we can leave it as is for consistency -- 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: 10 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: Sun, 12 Oct 2025 23:16:21 +0000 Gerrit-HasComments: Yes
