Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/23419 )
Change subject: KUDU-1261 [Java] Array Datatype client support ...................................................................... Patch Set 4: Code-Review+1 (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-kind nit: nested type ? http://gerrit.cloudera.org:8080/#/c/23419/4/java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java@279 PS4, Line 279: kind == Kind.ARRAY; Instead, could it be possible just to verify that descriptor.array isn't null? I guess the way how Descriptor to be extended for STRUCT and MAP assumes there will be fields of StructTypeDescriptor and MapTypeDescriptor. With that, it wouldn't be necessary to have the 'kind' at all. It actually serves no purpose here since there isn't a union-style data type in this Java approach, so there is no need to have a selector that would contain the information on what's actually is stored in the union type field. http://gerrit.cloudera.org:8080/#/c/23419/4/java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java@288 PS4, Line 288: static final class Descriptor Can this be a 'private static final' class? http://gerrit.cloudera.org:8080/#/c/23419/4/java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java@629 PS4, Line 629: this.nestedTypeDescriptor = null; If calling first 'array(true)' and then 'array(false)', tyhe type were set to NESTED, and the builder object would be in an unusable state. And there might be more cases of such a fallout because of stateful approach like this if there were other logical dependencies or extra verification logic. A stateful approach is always more complicated and brittle. Instead, is this possible to perform all the verification and populating the nestedTypeDescriptor field in the 'build()' method? 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 element DataType. nit: with the type of array elements ? 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(array) DataType column nit: for a nested column http://gerrit.cloudera.org:8080/#/c/23419/4/java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java@206 PS4, Line 206: defaultValue = pb.hasWriteDefaultValue() ? : byteStringToObject(elemType, typeAttributes, pb.getWriteDefaultValue()) : null; Shouldn't the default value for an array column be an array as well? Is there a test scenario with default value for an array column in a table that makes sure this works as expected, at least during the DDL phase? 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): ColumnSchema col = new ColumnSchema.ColumnSchemaBuilder("nested", Type.NESTED) .array(true) .build(); Does it makes sense to add a sub-scenario to be explicit on the result of such an attempt? 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: .*; nit: it seems there was some deliberate attempt to be specific with the imported symbols; does it make sense to keep it as-is and just add a few more instead of replacing it with this? http://gerrit.cloudera.org:8080/#/c/23419/4/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java@1178 PS4, Line 1178: assertTrue(returnedSchema.getColumn("int_arr").isArray()); nit: just for consistency, does it make sense to verify that the 'key' column isn't an array? http://gerrit.cloudera.org:8080/#/c/23419/4/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java@1188 PS4, Line 1188: float_arr nit: the name of the column doesn't seem to match the type; is it intentional? -- 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: 4 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: Sat, 20 Sep 2025 05:22:17 +0000 Gerrit-HasComments: Yes
