Alexey Serbin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/23483 )

Change subject: KUDU-1261 [Java] Add read support for Array Type
......................................................................


Patch Set 9:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/23483/9/java/kudu-client/src/main/java/org/apache/kudu/util/ArrayCellViewUtil.java
File java/kudu-client/src/main/java/org/apache/kudu/util/ArrayCellViewUtil.java:

http://gerrit.cloudera.org:8080/#/c/23483/9/java/kudu-client/src/main/java/org/apache/kudu/util/ArrayCellViewUtil.java@111
PS9, Line 111:   public static boolean[] toBoolArray(ArrayCellView view) {
             :     int n = view.length();
             :     boolean[] result = new boolean[n];
             :     for (int i = 0; i < n; i++) {
             :       if (!view.isValid(i)) {
             :         throw new IllegalStateException("Null element at " + i);
             :       }
             :       result[i] = view.getBoolean(i);
             :     }
             :     return result;
             :   }
             :
             :   // -------------------
             :   // Reference types
             :   // -------------------
             :
             :   public static String[] toStringArray(ArrayCellView view) {
             :     int n = view.length();
             :     String[] result = new String[n];
             :     for (int i = 0; i < n; i++) {
             :       result[i] = view.isValid(i) ? view.getString(i) : null;
             :     }
             :     return result;
             :   }
>From the API design standpoint, this isn't quite a consistent behavior across 
>different types: some toXxxArray throws exceptions on null elements, some are 
>not.

Since this implementation isn't CPU and memory efficient because of 
element-by-element copying anyway (especially knowing that there is flatbuffer 
introspection on every view.getXxx() call with XxxArray objects created), maybe 
these array-style getters should use boxed types for all the supported array 
types then?

At least, that would be consistent from the API standpoint, and practically 
usable, even if it's not efficient.

Otherwise, having this difference in behavior between, say, toBoolArray() and 
toStringArray() makes it inconsistent, while toXxxArray() methods that throw on 
NULL elements are practically unusable.

What do you think?



--
To view, visit http://gerrit.cloudera.org:8080/23483
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie26750cf540b0097c77e5454c1c1d20b3a194c52
Gerrit-Change-Number: 23483
Gerrit-PatchSet: 9
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: Zoltan Chovan <[email protected]>
Gerrit-Comment-Date: Sat, 04 Oct 2025 00:46:23 +0000
Gerrit-HasComments: Yes

Reply via email to