Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/23481 )
Change subject: KUDU-1261 [Java] Implement serdes of Array Type column ...................................................................... Patch Set 11: Code-Review+1 (12 comments) overall looks good to me, just a few nits, questions, and minor requests http://gerrit.cloudera.org:8080/#/c/23481/11/java/gradle/quality.gradle File java/gradle/quality.gradle: http://gerrit.cloudera.org:8080/#/c/23481/11/java/gradle/quality.gradle@36 PS11, Line 36: project.name != "kudu-flatbuffers" Just curious: do we run the checkstyle on protobuf-generated code as of now? http://gerrit.cloudera.org:8080/#/c/23481/11/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/23481/11/java/kudu-client/src/main/java/org/apache/kudu/client/Array1dSerdes.java@55 PS11, Line 55: getValues style nit: use field accessor-style method name instead, i.e. values()? http://gerrit.cloudera.org:8080/#/c/23481/11/java/kudu-client/src/main/java/org/apache/kudu/client/Array1dSerdes.java@59 PS11, Line 59: getValidity style nit: use field accessor-style method name instead, i.e. validity()? http://gerrit.cloudera.org:8080/#/c/23481/11/java/kudu-client/src/main/java/org/apache/kudu/client/Array1dSerdes.java@111 PS11, Line 111: Arrays.fill(validity, true); What is this for? Please add a comment. http://gerrit.cloudera.org:8080/#/c/23481/11/java/kudu-client/src/main/java/org/apache/kudu/client/Array1dSerdes.java@120 PS11, Line 120: Does it make sense to assert on the same length of validity and V? http://gerrit.cloudera.org:8080/#/c/23481/11/java/kudu-client/src/main/java/org/apache/kudu/client/Array1dSerdes.java@116 PS11, Line 116: // ---------------- Generic serialize/parse ---------------- : private static <V> byte[] serializePrimitive( : V values, boolean[] validity, int discriminator, : CreateVec<V> createVec, Start start, AddValues addValues, End end) { : : FlatBufferBuilder b = new FlatBufferBuilder(); : int vecOff = (values == null) ? 0 : createVec.apply(b, values); : int valOff = validityOffset(b, validity); : : start.apply(b); : if (vecOff != 0) { : addValues.apply(b, vecOff); : } : int arrOff = end.apply(b); : : int root = finishContent(b, discriminator, arrOff, valOff); : b.finish(root); : return b.sizedByteArray(); : } : : private static <A> ArrayResult parsePrimitive( : byte[] buf, int discriminator, : GetUnion<A> getUnion, ArrLen<A> lenFn, : NewArray newArray, Copier<A> copy) { : : Content c = readContent(buf); : if (c.dataType() != discriminator) { : throw new IllegalArgumentException("Unexpected union type: " + c.dataType()); : } : A arr = getUnion.apply(c); : int m = lenFn.apply(arr); : boolean[] validity = buildValidity(c, m); : : Object values = newArray.apply(m); : for (int i = 0; i < m; i++) { : copy.copy(arr, i, values); : } : : return new ArrayResult(values, validity); : } nit: move this to be along with corresponding functions for strings/binary in the end of the file? http://gerrit.cloudera.org:8080/#/c/23481/11/java/kudu-client/src/main/java/org/apache/kudu/client/Array1dSerdes.java@442 PS11, Line 442: readContent(buf) Does this work as expected if buf is null? http://gerrit.cloudera.org:8080/#/c/23481/11/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/23481/11/java/kudu-client/src/test/java/org/apache/kudu/client/TestArraySerdes.java@51 PS11, Line 51: Double Why not to use Float here? http://gerrit.cloudera.org:8080/#/c/23481/11/java/kudu-client/src/test/java/org/apache/kudu/client/TestArraySerdes.java@160 PS11, Line 160: // UINT16 Does it make sense to add test cases for INT16, INT32, and INT64 as well? I couldn't see them among the existing test cases. http://gerrit.cloudera.org:8080/#/c/23481/11/java/kudu-client/src/test/java/org/apache/kudu/client/TestArraySerdes.java@283 PS11, Line 283: } For at least one of the types, consider adding test cases that would verify serialization/de-serialization of: * empty arrays * null arrays (if null is accepted by Array1dSerdes.serializeXxx()) http://gerrit.cloudera.org:8080/#/c/23481/11/java/kudu-flatbuffers/build.gradle File java/kudu-flatbuffers/build.gradle: http://gerrit.cloudera.org:8080/#/c/23481/11/java/kudu-flatbuffers/build.gradle@26 PS11, Line 26: / nit: either remove this trailing symbol or add the same in the end for flatcOutputDir ? http://gerrit.cloudera.org:8080/#/c/23481/11/java/kudu-flatbuffers/build.gradle@41 PS11, Line 41: thirdparty/build Should it be looked up under thirdparty/installed/uninstrumented instead? At least, for the C++ code we use flatc from there. Once searching under thirdparty/installed/uninstrumented, the sanitizer-related stuff below might be removed. -- To view, visit http://gerrit.cloudera.org:8080/23481 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie1ad9f65fe94c8662ed0e0834ce849e078fc72d2 Gerrit-Change-Number: 23481 Gerrit-PatchSet: 11 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: Fri, 03 Oct 2025 01:05:23 +0000 Gerrit-HasComments: Yes
