Abhishek Chennaka 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 12: (15 comments) http://gerrit.cloudera.org:8080/#/c/23483/12/java/kudu-client/src/main/java/org/apache/kudu/client/ArrayCellView.java File java/kudu-client/src/main/java/org/apache/kudu/client/ArrayCellView.java: http://gerrit.cloudera.org:8080/#/c/23483/12/java/kudu-client/src/main/java/org/apache/kudu/client/ArrayCellView.java@46 PS12, Line 46: ArrayCellView > I didn't spot this in prior review rounds, but does it make sense to make t Done http://gerrit.cloudera.org:8080/#/c/23483/12/java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java File java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java: http://gerrit.cloudera.org:8080/#/c/23483/12/java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java@489 PS12, Line 489: Type type = col.getType(); > nit: move this to come after if (col.isArray()) {...} clause? Done http://gerrit.cloudera.org:8080/#/c/23483/12/java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java@626 PS12, Line 626: assert arr != null; > Could you use proper Precondition instead of assert here? Done http://gerrit.cloudera.org:8080/#/c/23483/12/java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java@667 PS12, Line 667: tsArr > What if tsAttr == null? It can’t be null because toTimestampArray() always allocates and returns a new Timestamp[], even for empty or all-null arrays. http://gerrit.cloudera.org:8080/#/c/23483/12/java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java@680 PS12, Line 680: > nit: add 'break' in 'default' as well? or that's prohibited by the style c Done http://gerrit.cloudera.org:8080/#/c/23483/12/java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java@775 PS12, Line 775: public final ArrayCellView getArray(int columnIndex) > Why do you want to expose ArrayCellView as a part of the public interface? Yes, agree with you. We can keep getArrayData() public and the users can access array elements though that method. http://gerrit.cloudera.org:8080/#/c/23483/12/java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java@794 PS12, Line 794: /** : * Convenience overload for {@link #getArray(int)} by column name. : */ : public final ArrayCellView getArray(String columnName) { : return getArray(this.schema.getColumnIndex(columnName)); : } > I don't think this should be a public method. Done http://gerrit.cloudera.org:8080/#/c/23483/12/java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java@845 PS12, Line 845: int precision = 0; : int scale = 0; : if (col.getTypeAttributes() != null) { : precision = col.getTypeAttributes().getPrecision(); : scale = col.getTypeAttributes().getScale(); : } : Type logicalType = col.getNestedTypeDescriptor().getArrayDescriptor().getElemType(); > Why not to do this in ArrayCellViewUtil method implementation instead, pass Done. We don't need check the size attribute for VARCHAR type during decoding. This is similar to the implementation we have for primitive VARCHAR type. http://gerrit.cloudera.org:8080/#/c/23483/12/java/kudu-client/src/main/java/org/apache/kudu/client/RowwiseRowResult.java File java/kudu-client/src/main/java/org/apache/kudu/client/RowwiseRowResult.java: http://gerrit.cloudera.org:8080/#/c/23483/12/java/kudu-client/src/main/java/org/apache/kudu/client/RowwiseRowResult.java@81 PS12, Line 81: if (column.isArray()) { : // Arrays are varlen on wire (offset + length), same as STRING/BINARY/VARCHAR. : previousSize = 16; : } else { : previousSize = column.getTypeSize(); : } > Is this needed as of PS12? Nope, not needed anymore. good catch. http://gerrit.cloudera.org:8080/#/c/23483/10/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/10/java/kudu-client/src/main/java/org/apache/kudu/util/ArrayCellViewUtil.java@59 PS10, Line 59: Boxed num > very small nit (I don't want to be pedantic): if it's boxed then it's a ref Fixed it. http://gerrit.cloudera.org:8080/#/c/23483/12/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/23483/12/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java@2172 PS12, Line 2172: @MasterServerConfig(flags = { : "--cfile_support_arrays=true", : }) > I guess this isn't needed with http://gerrit.cloudera.org:8080/23479 Done http://gerrit.cloudera.org:8080/#/c/23483/12/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java@2235 PS12, Line 2235: if (i % 5 == 0) { : session.flush(); : } > What is this for? There flush() call at line 2239 already. Multiple flushes to catch any potential bugs during client side serialization or encoding across multiple RPC calls. We can get rid of it if you think it doesn't make sense but I don't see any harm in it :) http://gerrit.cloudera.org:8080/#/c/23483/12/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java@2245 PS12, Line 2245: // Ensure deterministic order by sorting on the key column : rowStrings.sort(Comparator.comparingInt(rowStr -> { : int eq = rowStr.indexOf('='); : int comma = rowStr.indexOf(',', eq); : return Integer.parseInt(rowStr.substring(eq + 1, comma).trim()); : })); > I'm not sure I understand why this is necessary: scanTableToStrings() alrea Good catch, I think I added it in when I was writing 50 rows in earlier versions and forgot to remove this later on. http://gerrit.cloudera.org:8080/#/c/23483/12/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java@2311 PS12, Line 2311: if (intArr != null) { > Here and below: should there be an assert based on index of the row instead Done http://gerrit.cloudera.org:8080/#/c/23483/12/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java@2314 PS12, Line 2314: } > Here and below: consider adding verification of the actual values retrieved Done -- 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: 12 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: Thu, 16 Oct 2025 00:48:21 +0000 Gerrit-HasComments: Yes
