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 7: Code-Review+1

(2 comments)

Looks OK.  However, the LINT isn't happy:

  Java Gradle check failed

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@279
PS4, Line 279:
> But updated it with the given suggestions. Let me know what you think.

Thanks, this looks OK.

> switch (nestedType.getKind()) {
>  case ARRAY: return nestedType.getArrayDescriptor();
>  case MAP: return nestedType.getMapDescriptor();
>  case STRUCT: ...
> }

I might be mission something, but I don't see where such a code would be needed.

As for instanceof: it wouldn't make sense since the fields of the Descriptor 
class are expected to be of different types: there is not a union type in Java. 
 Whatever field isn't null, that's corresponds to the type of the nested type.

Anyways, it's always possible to update it as needed when it's clear what the 
needs are :)  Since we aren't talking about publicly-facing API, no future 
proofing effort is warranted, IMHO.


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@206
PS4, Line 206:       // TODO(achennaka): Re-visit the read and write default 
implementation for nested types.
             :       defaultValue = pb.hasWriteDefaultValue() ?
> Added a TODO for default value handling in followup changelist.
Yes, given current stage of this effort, it should be good enough.



--
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: 7
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 21:23:47 +0000
Gerrit-HasComments: Yes

Reply via email to