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 5: (6 comments) http://gerrit.cloudera.org:8080/#/c/23482/5/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/5/java/kudu-client/src/main/java/org/apache/kudu/client/ArrayCellView.java@46 PS5, Line 46: ArrayCellView the following methods are not used anywhere: getUInt8() getUInt16() getUInt32() getUInt64() getDate() getTimestamp() which means they are lacking the corresponding tests in TestPartialRow http://gerrit.cloudera.org:8080/#/c/23482/5/java/kudu-client/src/main/java/org/apache/kudu/client/ArrayCellView.java@97 PS5, Line 97: (byte) Why is this cast necessary? The generated Int8Array generated class already returns a byte: public byte values(int j) { int o = __offset(4); return o != 0 ? bb.get(__vector(o) + j * 1) : 0; } http://gerrit.cloudera.org:8080/#/c/23482/5/java/kudu-client/src/main/java/org/apache/kudu/client/ArrayCellView.java@116 PS5, Line 116: (short) same question as above, it this cast necessary? http://gerrit.cloudera.org:8080/#/c/23482/5/java/kudu-client/src/main/java/org/apache/kudu/client/ArrayCellView.java@207 PS5, Line 207: (byte) elem.values(j); UInt8Array.values() returns an int, so this cast is only safe if the value is always between -128 to 127, since this deals with binary values, this shouldn't happen, but if for whatever reason there's a scrambled data here, would the resulting -1 as a binary value cause any issues down the line? Do you think a validation check is warranted here? http://gerrit.cloudera.org:8080/#/c/23482/5/java/kudu-client/src/main/java/org/apache/kudu/client/ArrayCellView.java@233 PS5, Line 233: elementWireFamily this method shows up as unused, is it needed in this change? http://gerrit.cloudera.org:8080/#/c/23482/5/java/kudu-client/src/main/java/org/apache/kudu/client/ArrayCellView.java@285 PS5, Line 285: switch I'm seeing a lot of code repetition in this switch-case, maybe consider extracting the shared parts to a helper function? -- 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: 5 Gerrit-Owner: 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, 02 Oct 2025 12:47:47 +0000 Gerrit-HasComments: Yes
