Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/20749 )
Change subject: KUDU-1261 Java client complex type introduction ...................................................................... Patch Set 4: (7 comments) http://gerrit.cloudera.org:8080/#/c/20749/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20749/4//COMMIT_MSG@7 PS4, Line 7: KUDU-1261 Java client complex type introduction > Is this a WIP patch? If so, maybe mark it so. Done http://gerrit.cloudera.org:8080/#/c/20749/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/20749/4/java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java@40 PS4, Line 40: private final Type type; : private ComplexTypeArray complexTypeArray = null; > Just curious: did you explore an option to extend Type to cover the case of The idea was to have a separate class for each complex type but as discussed I am pushing a new revision of the patch which should address this concern. http://gerrit.cloudera.org:8080/#/c/20749/4/java/kudu-client/src/main/java/org/apache/kudu/ComplexTypeArray.java File java/kudu-client/src/main/java/org/apache/kudu/ComplexTypeArray.java: http://gerrit.cloudera.org:8080/#/c/20749/4/java/kudu-client/src/main/java/org/apache/kudu/ComplexTypeArray.java@6 PS4, Line 6: /** : * Constructor used to create the Complex Type : * : * @param type type of the elements stored in this complex type : */ : ComplexTypeArray(Type type) { : this.type = type; : } : : public Type getType() { : return this.type; : } > This seems to be nothing but a trivial wrapper around Type. If so, why not Done http://gerrit.cloudera.org:8080/#/c/20749/4/src/kudu/common/common.proto File src/kudu/common/common.proto: http://gerrit.cloudera.org:8080/#/c/20749/4/src/kudu/common/common.proto@105 PS4, Line 105: ComplexTypeArrayPB > If the idea is to provide just the type of the elements in the array, why t Yes this was a way of future proofing to add fields like the nested level and any other information needed when implementing other complex types. http://gerrit.cloudera.org:8080/#/c/20749/4/src/kudu/common/common.proto@106 PS4, Line 106: required > Make this field optional: Done http://gerrit.cloudera.org:8080/#/c/20749/4/src/kudu/common/common.proto@106 PS4, Line 106: Type > nit: field names should start with non-capital letters Done http://gerrit.cloudera.org:8080/#/c/20749/4/src/kudu/common/common.proto@151 PS4, Line 151: complexTypeArray > style nit: use snake_case, not camelCase for the field names in proto files Done -- To view, visit http://gerrit.cloudera.org:8080/20749 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I92825c4ab430ccd4bbd902b06679488af91d9804 Gerrit-Change-Number: 20749 Gerrit-PatchSet: 4 Gerrit-Owner: Abhishek Chennaka <achenn...@cloudera.com> Gerrit-Reviewer: Abhishek Chennaka <achenn...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <ale...@apache.org> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy <mre...@cloudera.com> Gerrit-Comment-Date: Fri, 21 Jun 2024 20:44:06 +0000 Gerrit-HasComments: Yes
