Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/15602 )
Change subject: tserver: add support for returning scan result in columnar layout ...................................................................... Patch Set 3: (11 comments) http://gerrit.cloudera.org:8080/#/c/15602/2/src/kudu/tserver/tablet_server-test.cc File src/kudu/tserver/tablet_server-test.cc: http://gerrit.cloudera.org:8080/#/c/15602/2/src/kudu/tserver/tablet_server-test.cc@2543 PS2, Line 2543: TEST_F(TabletServerTest, TestColumnarScan) { > nit: Consider extending this to return multiple batches to exercise the con I'm going to cover this in the test that uses the client -- is that OK? http://gerrit.cloudera.org:8080/#/c/15602/2/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/15602/2/src/kudu/tserver/tablet_service.cc@743 PS2, Line 743: : // Add the given RowBlock to the pending response. : virtual int SerializeRowBlock(const RowBlock& row_block, : const Schema* client_projection_schema) = 0; : > nit: doc these? Done http://gerrit.cloudera.org:8080/#/c/15602/2/src/kudu/tserver/tablet_service.cc@791 PS2, Line 791: resp- > nit: add a 'done_' here too? Done http://gerrit.cloudera.org:8080/#/c/15602/2/src/kudu/tserver/tablet_service.cc@803 PS2, Line 803: Row > nit: Can we also validate the PAD_UNIX_MICROS flag here? the above checks that no flags other than COLUMNAR_LAYOUT have been set http://gerrit.cloudera.org:8080/#/c/15602/2/src/kudu/tserver/tablet_service.cc@911 PS2, Line 911: int64_t NumRowsReturned() const override { > warning: do not use 'else' after 'return' [readability-else-after-return] Done http://gerrit.cloudera.org:8080/#/c/15602/2/src/kudu/tserver/tablet_service.cc@913 PS2, Line 913: } > nit: move this down so we have an explicit return? Done http://gerrit.cloudera.org:8080/#/c/15602/2/src/kudu/tserver/tablet_service.cc@2214 PS2, Line 2214: eturn; > Should we also update TabletServiceAdminImpl? I don't think so, since this feature is only used for calls in the TabletServiceImpl, right? I'm not sure why we have the other features present in the TabletServiceAdminImpl::SupportsFeature() function, it seems like they aren't relevant http://gerrit.cloudera.org:8080/#/c/15602/2/src/kudu/tserver/tablet_service.cc@2731 PS2, Line 2731: LOG(INFO) << Substitute("Scan: $0: call sequence id=$1, remote=$2", : s.ToString(), req->call_seq_id(), rpc_context->requestor_string()); : *error_code = code; : return s; : } : > Should we move this down under the ScopedUnregisterScanner? If this fails, Done http://gerrit.cloudera.org:8080/#/c/15602/2/src/kudu/tserver/tserver.proto File src/kudu/tserver/tserver.proto: http://gerrit.cloudera.org:8080/#/c/15602/2/src/kudu/tserver/tserver.proto@390 PS2, Line 390: // The error, if an error > nit: Should this be in wire_protocol.proto alongside RowwiseRowBlockPB? Done http://gerrit.cloudera.org:8080/#/c/15602/2/src/kudu/tserver/tserver.proto@391 PS2, Line 391: optional TabletS > nit: Can you note the units here are sidecar indices? Done http://gerrit.cloudera.org:8080/#/c/15602/2/src/kudu/tserver/tserver.proto@394 PS2, Line 394: // to pull new rows from the scanner. > nit: Maybe also note the relation between this and 'data_sidecar' re offset Done -- To view, visit http://gerrit.cloudera.org:8080/15602 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib99faa27554ec228492f096bb8760c883e44660d Gerrit-Change-Number: 15602 Gerrit-PatchSet: 3 Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Tue, 31 Mar 2020 17:39:22 +0000 Gerrit-HasComments: Yes
