[kudu-CR] KUDU-1261 Java client complex type introduction

["Abhishek Chennaka (Code Review)" <ge...@cloudera.org>]

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 <ac...@cloudera.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Fri, 21 Jun 2024 20:44:06 +0000
Gerrit-HasComments: Yes