Abhishek Chennaka 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 12: (14 comments) 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: > Just curious: do we run the checkstyle on protobuf-generated code as of now No, the difference comes from the sourceSets { java { } } block in the kudu-flatbuffers build.gradle, which causes Checkstyle to assume there are Java sources to analyze. In the case of, kudu-proto uses sourceSets { proto { } }, so sourceSets.main.java remains empty. As a result, Checkstyle has nothing to process in kudu-proto and is skipped. I moved the exception to kudu-flatbuffers/build.gradle to keep things cleaner. http://gerrit.cloudera.org:8080/#/c/23481/8/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/8/java/kudu-client/src/main/java/org/apache/kudu/client/Array1dSerdes.java@134 PS8, Line 134: > I may be misunderstanding something, but shouldn't this be an unexpected da That is referring to the union defined in https://github.com/apache/kudu/blob/95abb8d6525d093b5961a7b9f2dc8c6193a47c33/src/kudu/common/serdes/array1d.fbs#L37 http://gerrit.cloudera.org:8080/#/c/23481/8/java/kudu-client/src/main/java/org/apache/kudu/client/Array1dSerdes.java@192 PS8, Line 192: > is this a todo for later? Ah, addressed it. Thanks! 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: > I don't necessarily agree, getters and setters (getX, setX) are pretty much Thanks Zoltan. Yes, that is the standard followed in Kudu Java codebase. http://gerrit.cloudera.org:8080/#/c/23481/11/java/kudu-client/src/main/java/org/apache/kudu/client/Array1dSerdes.java@59 PS11, Line 59: > style nit: use field accessor-style method name instead, i.e. validity()? Same as above 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. Done 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? Yes, done http://gerrit.cloudera.org:8080/#/c/23481/11/java/kudu-client/src/main/java/org/apache/kudu/client/Array1dSerdes.java@116 PS11, Line 116: String.format("Invalid validity length %d (expected %d)", n, m)); : } : for (int i = 0; i < m; i++) { : validity[i] = c.validity(i); : } : return validity; : } : : // ---------------- Int8 ---------------- : public static byte[] serializeInt8(byte[] values, boolean[] validity) { : return serializePrimitive( : values, validity, ScalarArray.Int8Array, : Int8Array::createValuesVector, : Int8Array::startInt8Array, : Int8Array::addValues, : Int8Array::endInt8Array : ); : } : : public static ArrayResult parseInt8(byte[] buf) { : return parsePrimitive( : buf, ScalarArray.Int8Array, : c -> (Int8Array) c.data(new Int8Array()), : Int8Array::valuesLength, : byte[]::new, : (a, i, out) -> ((byte[]) out)[i] = a.values(i) : ); : } : : // ---------------- UInt8 ---------------- : public static byte[] serializeUInt8(byte[] values, boolean[] validity) { : byte[] narrowed = null; : if (values != null) { : narrowed = new byte[values.length]; : for (int i = 0; i < values.length; i++) { : narrowed[i] = (byte)(values[i] & 0xFF); : } : } : return serializePrimitive( : > nit: move this to be along with corresponding functions for strings/binary Done http://gerrit.cloudera.org:8080/#/c/23481/11/java/kudu-client/src/main/java/org/apache/kudu/client/Array1dSerdes.java@442 PS11, Line 442: > Does this work as expected if buf is null? Throws NPE. Added a message to make it clear. 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: float[ > Why not to use Float here? Done http://gerrit.cloudera.org:8080/#/c/23481/11/java/kudu-client/src/test/java/org/apache/kudu/client/TestArraySerdes.java@160 PS11, Line 160: // ---------------------------- > Does it make sense to add test cases for INT16, INT32, and INT64 as well? They are present above. I switched the order to make it more reader friendly. 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 Done 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 flat Done http://gerrit.cloudera.org:8080/#/c/23481/11/java/kudu-flatbuffers/build.gradle@41 PS11, Line 41: > Should it be looked up under thirdparty/installed/uninstrumented instead? Good point, that simplified the code too. Thanks! -- 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: 12 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 19:56:08 +0000 Gerrit-HasComments: Yes
