Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/23563 )
Change subject: KUDU-1261 [Java] Add PartialRow.getArrayData() ...................................................................... Patch Set 3: Code-Review+1 (3 comments) overall looks good to me, just a few nits and questions http://gerrit.cloudera.org:8080/#/c/23563/3/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/23563/3/java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java@955 PS3, Line 955: public final Object getArrayData(int columnIndex) { nit: add the Nullable annotation? http://gerrit.cloudera.org:8080/#/c/23563/3/java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java@956 PS3, Line 956: Type.NESTED nit: in addition to verifying the type of the column cell, does it make sense to check for the proper nested array descriptor? http://gerrit.cloudera.org:8080/#/c/23563/3/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/23563/3/java/kudu-client/src/test/java/org/apache/kudu/client/TestPartialRow.java@309 PS3, Line 309: String[] arr = (String[]) view.toJavaArray(schema.getColumn("strings")); Is it possible to unify this across all the various scenarios? If not, please add a comment explaining why the case of String arrays is so special. -- To view, visit http://gerrit.cloudera.org:8080/23563 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I55e7ecfc6876c8aba5f15232d570617ad008f772 Gerrit-Change-Number: 23563 Gerrit-PatchSet: 3 Gerrit-Owner: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 21 Oct 2025 01:27:25 +0000 Gerrit-HasComments: Yes
