Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/15983 )
Change subject: Support columnar layout row format flag and result set iteration in Java client. ...................................................................... Patch Set 3: (5 comments) Thanks for the contribution! I did a quick overview and gave some high level feedback. Let me know what you think. http://gerrit.cloudera.org:8080/#/c/15983/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/15983/3//COMMIT_MSG@7 PS3, Line 7: Support columnar layout row format flag and result set iteration in Java client. nit: prepend "[java]" to indicate this change is for the java client. http://gerrit.cloudera.org:8080/#/c/15983/3/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java: http://gerrit.cloudera.org:8080/#/c/15983/3/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java@502 PS3, Line 502: public void setRowFormatFlags(RowFormatFlags rowFormatFlag) { Instead of exposing RowFormatFlags, I think we should instead add an enum for the formats we intend to support (ROW/COLUMN) and use that here to set the rowFormatFlags for the RPC message. That allows us to decouple wire format details from the public client API. We can default to ROW for now and consider defaulting to COLUMN in the future. In AsyncKuduScanner.createRequestPB it would look something like: long rowFormatFlags = Tserver.RowFormatFlags.NO_FLAGS_VALUE; if (dataFormat == DataFormat.COLUMN) { rowFormatFlags |= Tserver.RowFormatFlags.COLUMNAR_LAYOUT.getNumber(); } newBuilder.setRowFormatFlags(rowFormatFlags); http://gerrit.cloudera.org:8080/#/c/15983/3/java/kudu-client/src/main/java/org/apache/kudu/client/ColumnarRowResultIterator.java File java/kudu-client/src/main/java/org/apache/kudu/client/ColumnarRowResultIterator.java: http://gerrit.cloudera.org:8080/#/c/15983/3/java/kudu-client/src/main/java/org/apache/kudu/client/ColumnarRowResultIterator.java@97 PS3, Line 97: int numRows = (int)data.getNumRows(); We should use import static java.lang.Math.toIntExact to ensure we don't truncate a value that is too large. http://gerrit.cloudera.org:8080/#/c/15983/3/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/15983/3/java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java@40 PS3, Line 40: public abstract class RowResult { I am not sure we can make this it's abstract given it's a public interface. Instead could we push the columnar/row-base logic into this RowResult implementation? Most of the logic here is shared (type validation, documentation, etc). Perhaps using composition instead of inheritance could work. I think it might be possible to have a `getRawData` method that can return the correct byte[] for both columnar and row based data. Additionally a similar `getDataOffset` method could be added to handle calculating the offset into the array. The results of those methods can then be passed to Bytes.Get*() calls. I haven't full worked through this so there might be some unforeseen snags. http://gerrit.cloudera.org:8080/#/c/15983/3/java/kudu-client/src/main/java/org/apache/kudu/client/RowResultIterator.java File java/kudu-client/src/main/java/org/apache/kudu/client/RowResultIterator.java: http://gerrit.cloudera.org:8080/#/c/15983/3/java/kudu-client/src/main/java/org/apache/kudu/client/RowResultIterator.java@34 PS3, Line 34: public abstract class RowResultIterator extends KuduRpcResponse implements Iterator<RowResult>, Given this is a public class, I don't think we can change it to be abstract. Instead I think we could handle the format switching functionality within the RowResultIterator itself. I think this makes sense given there is already a static factory method. We can push more of the handling into the RowResult itself too. -- To view, visit http://gerrit.cloudera.org:8080/15983 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I580c9bb48e797ca8de9d88e7892df6860eed7070 Gerrit-Change-Number: 15983 Gerrit-PatchSet: 3 Gerrit-Owner: Shuping Zhou <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Tue, 26 May 2020 21:34:16 +0000 Gerrit-HasComments: Yes
