Alexey Serbin 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: (14 comments) 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@60 PS10, Line 60: /** Return the underlying FlatBuffer bytes exactly as passed in. */ : public byte[] toBytes() { : return rawBytes; : } Is it used anywhere? http://gerrit.cloudera.org:8080/#/c/23482/10/java/kudu-client/src/main/java/org/apache/kudu/client/ArrayCellView.java@73 PS10, Line 73: IndexOutOfBoundsException() Does it make sense to provide information on effective values of i and content.validityLength() with the exception? http://gerrit.cloudera.org:8080/#/c/23482/10/java/kudu-client/src/main/java/org/apache/kudu/client/ArrayCellView.java@79 PS10, Line 79: Typed accessors nit: Types single element accessors http://gerrit.cloudera.org:8080/#/c/23482/10/java/kudu-client/src/main/java/org/apache/kudu/client/ArrayCellView.java@86 PS10, Line 86: "Element " + i + " is NULL" here and elsewhere: is it possible to use a constant with the format of the message, and re-use it everywhere below? http://gerrit.cloudera.org:8080/#/c/23482/10/java/kudu-client/src/main/java/org/apache/kudu/client/ArrayCellView.java@111 PS10, Line 111: & 0xFF Looking into the UInt8Array.values(int j) implementation, I can see it already does bit-wise AND with 0xFF. Can we drop this extra bit-wise AND with 0xFF then? http://gerrit.cloudera.org:8080/#/c/23482/10/java/kudu-client/src/main/java/org/apache/kudu/client/ArrayCellView.java@132 PS10, Line 132: & 0xFFFF Looking into the UInt16Array.values(int j) implementation, I can see it already does bit-wise AND with 0xFFFF. Can we drop this extra bit-wise AND with 0xFFFF then? http://gerrit.cloudera.org:8080/#/c/23482/10/java/kudu-client/src/main/java/org/apache/kudu/client/ArrayCellView.java@154 PS10, Line 154: & 0xFFFFFFFFL Same here: UInt32Array.values(int j) already does this, so this isn't needed? http://gerrit.cloudera.org:8080/#/c/23482/10/java/kudu-client/src/main/java/org/apache/kudu/client/ArrayCellView.java@267 PS10, Line 267: & 0xFF not needed? http://gerrit.cloudera.org:8080/#/c/23482/10/java/kudu-client/src/main/java/org/apache/kudu/client/ArrayCellView.java@279 PS10, Line 279: & 0xFFFF not needed? http://gerrit.cloudera.org:8080/#/c/23482/10/java/kudu-client/src/main/java/org/apache/kudu/client/ArrayCellView.java@291 PS10, Line 291: & 0xFFFFFFFFL not needed? http://gerrit.cloudera.org:8080/#/c/23482/10/java/kudu-client/src/main/java/org/apache/kudu/client/ArrayCellView.java@348 PS10, Line 348: "Not a " + what + " array (tag=" + typeTag + ")" ScalarArray has name() method, so I'd rather see the expected name of the type/tag, the expected tag number, and the actual tag number. I'd rather not have 'what' because at this level it's not about Kudu column types, rather about types of on-the-wire messages. 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 something that lets reader know this is something about stringifying arrays? http://gerrit.cloudera.org:8080/#/c/23482/10/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/10/java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java@1465 PS10, Line 1465: * The accepted Object type is based on the column's {@link Type}: Should this be updated to reflect on the recent updates? http://gerrit.cloudera.org:8080/#/c/23482/10/java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java@1545 PS10, Line 1545: null How is it going to work if validity array is null? Does it have some special meaning, similar to all bits set true? I guess it makes sense documenting this. -- 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: Sat, 04 Oct 2025 06:06:07 +0000 Gerrit-HasComments: Yes
