Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/23419 )
Change subject: KUDU-1261 [Java] Array Datatype client support ...................................................................... Patch Set 5: (11 comments) http://gerrit.cloudera.org:8080/#/c/23419/4/java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java File java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java: http://gerrit.cloudera.org:8080/#/c/23419/4/java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java@266 PS4, Line 266: nested type > nit: nested type ? Done http://gerrit.cloudera.org:8080/#/c/23419/4/java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java@279 PS4, Line 279: > Instead, could it be possible just to verify that descriptor.array isn't nu Yes that is redundant in currently but more of a future proofing for future data types. For example: switch (nestedType.getKind()) { case ARRAY: return nestedType.getArrayDescriptor(); case MAP: return nestedType.getMapDescriptor(); case STRUCT: ... } Without this we need to introduce something like: instanceof checks (if (descriptor instanceof ArrayTypeDescriptor) …) — which is a little messy ======================== But updated it with the given suggestions. Let me know what you think. http://gerrit.cloudera.org:8080/#/c/23419/4/java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java@288 PS4, Line 288: final ArrayTypeDescriptor a > Can this be a 'private static final' class? Done http://gerrit.cloudera.org:8080/#/c/23419/4/java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java@629 PS4, Line 629: */ > If calling first 'array(true)' and then 'array(false)', tyhe type were set Yes, good point. Done http://gerrit.cloudera.org:8080/#/c/23419/4/java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java File java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java: http://gerrit.cloudera.org:8080/#/c/23419/4/java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java@137 PS4, Line 137: with the type of array > nit: with the type of array elements ? Done http://gerrit.cloudera.org:8080/#/c/23419/4/java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java@200 PS4, Line 200: for nested column. > nit: for a nested column Done http://gerrit.cloudera.org:8080/#/c/23419/4/java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java@206 PS4, Line 206: // TODO(achennaka): Re-visit the read and write default implementation for nested types. : defaultValue = pb.hasWriteDefaultValue() ? > Shouldn't the default value for an array column be an array as well? Added a TODO for default value handling in followup changelist. http://gerrit.cloudera.org:8080/#/c/23419/4/java/kudu-client/src/test/java/org/apache/kudu/TestColumnSchema.java File java/kudu-client/src/test/java/org/apache/kudu/TestColumnSchema.java: http://gerrit.cloudera.org:8080/#/c/23419/4/java/kudu-client/src/test/java/org/apache/kudu/TestColumnSchema.java@212 PS4, Line 212: ColumnSchema varcharArrayCol = new ColumnSchema.ColumnSchemaBuilder("varchar_arr", Type.VARCHAR) : .typeAttributes(varcharAttrs) : .array(true) : .nullable(true) : .build(); > BTW, what happens if trying to do the following (e.g., it might be a typo): Done http://gerrit.cloudera.org:8080/#/c/23419/4/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/23419/4/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java@25 PS4, Line 25: .co > nit: it seems there was some deliberate attempt to be specific with the imp Fixed it. http://gerrit.cloudera.org:8080/#/c/23419/4/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java@1178 PS4, Line 1178: * Test creating a table schema with an array type column. > nit: just for consistency, does it make sense to verify that the 'key' colu Done http://gerrit.cloudera.org:8080/#/c/23419/4/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java@1188 PS4, Line 1188: test) > nit: the name of the column doesn't seem to match the type; is it intention Fixed it -- To view, visit http://gerrit.cloudera.org:8080/23419 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I02b5fc52d984d424b7be53e3cc4e3236a8d33aa9 Gerrit-Change-Number: 23419 Gerrit-PatchSet: 5 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: Xuebin Su <[email protected]> Gerrit-Comment-Date: Tue, 23 Sep 2025 01:57:21 +0000 Gerrit-HasComments: Yes
