Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/15660 )
Change subject: wire_protocol: change columnar serialization of varlen data to match Arrow ...................................................................... Patch Set 3: (7 comments) http://gerrit.cloudera.org:8080/#/c/15660/3/src/kudu/client/columnar_scan_batch.h File src/kudu/client/columnar_scan_batch.h: http://gerrit.cloudera.org:8080/#/c/15660/3/src/kudu/client/columnar_scan_batch.h@45 PS3, Line 45: but without the alignment and padding guarantees that are made by : /// the Arrow IPC serialization. > nit: just to be sure, is this to say that the guarantees are only important well, they might be important for in-memory on some architectures as well (eg ARM) but right now it's not guaranteed. In order to guarantee it we'd need to do some work in our RPC system to be sure that our first sidecar arrives on an aligned memory boundary and then each successive sidecar is padded appropriately, etc. It may be worth doing, but it's work not yet done. http://gerrit.cloudera.org:8080/#/c/15660/3/src/kudu/client/scanner-internal.cc File src/kudu/client/scanner-internal.cc: http://gerrit.cloudera.org:8080/#/c/15660/3/src/kudu/client/scanner-internal.cc@882 PS3, Line 882: resp_data_.columns(idx) > nit: why not use `col`? mistake.. fixed http://gerrit.cloudera.org:8080/#/c/15660/3/src/kudu/common/columnar_serialization.cc File src/kudu/common/columnar_serialization.cc: http://gerrit.cloudera.org:8080/#/c/15660/3/src/kudu/common/columnar_serialization.cc@513 PS3, Line 513: total_size > nit: seems a little odd to call this the total size, given old_size + total went with total_added_size http://gerrit.cloudera.org:8080/#/c/15660/3/src/kudu/common/columnar_serialization.cc@615 PS3, Line 615: SelectedRows sel = block.selection_vector()->GetSelectedRows(); > Can we short circuit if we selected 0 rows in this row block? If so, maybe Added the short circuit, but not sure about the dcheck. Is there sometihng about those calls that you tink is unsafe if n_sel is 0? i think they'd all fall through and do nothing in the correct way http://gerrit.cloudera.org:8080/#/c/15660/3/src/kudu/common/rowblock.h File src/kudu/common/rowblock.h: http://gerrit.cloudera.org:8080/#/c/15660/3/src/kudu/common/rowblock.h@265 PS3, Line 265: num_selected > nit: could use sel_vector_->nrows() to avoid the extra all_selected_ evalua gonna disagree on this one -- the compiler will see that the load of all_selected_ is redundant and inline it anyway, and I think this is more clear / less error-prone http://gerrit.cloudera.org:8080/#/c/15660/3/src/kudu/common/wire_protocol-test.cc File src/kudu/common/wire_protocol-test.cc: http://gerrit.cloudera.org:8080/#/c/15660/3/src/kudu/common/wire_protocol-test.cc@341 PS3, Line 341: sizeof(uint32_t)*dst_row_idx > nit: spacing Done http://gerrit.cloudera.org:8080/#/c/15660/3/src/kudu/common/wire_protocol.proto File src/kudu/common/wire_protocol.proto: http://gerrit.cloudera.org:8080/#/c/15660/3/src/kudu/common/wire_protocol.proto@161 PS3, Line 161: num_rows+1 > nit: for consistency with elsewhere in this patch, separate these with spac Done -- To view, visit http://gerrit.cloudera.org:8080/15660 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iadf728744feb83f5980e62bea4fd7634a1a52467 Gerrit-Change-Number: 15660 Gerrit-PatchSet: 3 Gerrit-Owner: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Andrew Wong <andrew.w...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Tue, 07 Apr 2020 17:11:47 +0000 Gerrit-HasComments: Yes
