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 17: (4 comments) http://gerrit.cloudera.org:8080/#/c/23482/17/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/17/java/kudu-client/src/main/java/org/apache/kudu/client/Array1dSerdes.java@83 PS17, Line 83: length I'd suggest removing the code related to 'length' from this method: it does not look like it belongs in here. If there is a need to have some validation of values and validity vector, put this length/validity verification in there as well? Maybe, consolidate it with verification of data elements and the validity vector, so it would be together with the code like below: if (validity == null || validity.length == 0) { if (hasNulls(values)) { throw new IllegalArgumentException( "Empty validity vector not allowed when values contain nulls"); } } http://gerrit.cloudera.org:8080/#/c/23482/17/java/kudu-client/src/main/java/org/apache/kudu/client/Array1dSerdes.java@392 PS17, Line 392: (would be indistinguishable) nit for here and below: indistinguishable from what? http://gerrit.cloudera.org:8080/#/c/23482/17/java/kudu-client/src/main/java/org/apache/kudu/client/Array1dSerdes.java@491 PS17, Line 491: for (int i = 0; i < valueLen; i++) { : Object elem = Array.get(values, i); : if (elem == null) { nit: is it possible to use 'hasNulls()' here as well? http://gerrit.cloudera.org:8080/#/c/23482/17/java/kudu-client/src/test/java/org/apache/kudu/client/TestArraySerdes.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestArraySerdes.java: http://gerrit.cloudera.org:8080/#/c/23482/17/java/kudu-client/src/test/java/org/apache/kudu/client/TestArraySerdes.java@377 PS17, Line 377: byte[] buf1 = Array1dSerdes.serializeInt32(new int[]{1,2,3}, null); Please also validate how it works with non-null empty validity array. Also, since Array1dSerdes has different implementation for serializing different types, consider adding tests for all the different types in addition to this one which is covered already. I.e. add test to cover the code in serializeBinary and serializeString in addition to already covered serializePrimitives. Thanks! -- 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: 17 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: Wed, 22 Oct 2025 23:51:52 +0000 Gerrit-HasComments: Yes
