Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/23483 )
Change subject: KUDU-1261 [Java] Add read support for Array Type ...................................................................... Patch Set 13: Code-Review+1 (5 comments) Overall looks OK to me, just a few nits. As discussed, I'm going to take another look as you revv this patch: I guess there will be more crucial updates on the API itself. Thanks a lot for working on this! http://gerrit.cloudera.org:8080/#/c/23483/13/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/23483/13/java/kudu-client/src/main/java/org/apache/kudu/client/ArrayCellView.java@75 PS13, Line 75: * <p>If no validity bitmap is present, this corresponds to the : * underlying values vector length.</p> nit: I'm not sure this brings in more clarity, at least to me it looked a bit confusing http://gerrit.cloudera.org:8080/#/c/23483/13/java/kudu-client/src/main/java/org/apache/kudu/client/ArrayCellViewHelper.java File java/kudu-client/src/main/java/org/apache/kudu/client/ArrayCellViewHelper.java: http://gerrit.cloudera.org:8080/#/c/23483/13/java/kudu-client/src/main/java/org/apache/kudu/client/ArrayCellViewHelper.java@270 PS13, Line 270: nit: fix the indent? http://gerrit.cloudera.org:8080/#/c/23483/13/java/kudu-client/src/main/java/org/apache/kudu/client/ArrayCellViewHelper.java@327 PS13, Line 327: and (future) DECIMAL128 both use BinaryArray nit: I'm not sure this will hold true since it depends on future implementation of INT128 support in flatbuffers, so I'd suggest refraining of future-looking statements like this http://gerrit.cloudera.org:8080/#/c/23483/12/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java: http://gerrit.cloudera.org:8080/#/c/23483/12/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java@2235 PS12, Line 2235: session.flush(); : : // > Multiple flushes to catch any potential bugs during client side serializati This is one extra flush, not multiple given there are 10 rows total. As for serialization of array cells, it's done on per-element basis, and it isn't related to flushing data to the server side. From the other side, if there is a bug somewhere in flushing/sending out the data to the server, it would pop up elsewhere as well, and we have special test cases to cover that. OK, if you want to keep it here, that's fine, but I don't see how it adds any value into this test scenario. http://gerrit.cloudera.org:8080/#/c/23483/12/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java@2314 PS12, Line 2314: } > Done Thank you for addressing this! -- To view, visit http://gerrit.cloudera.org:8080/23483 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie26750cf540b0097c77e5454c1c1d20b3a194c52 Gerrit-Change-Number: 23483 Gerrit-PatchSet: 13 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, 16 Oct 2025 23:35:47 +0000 Gerrit-HasComments: Yes
