Dan Burkert has posted comments on this change. Change subject: [c++-client] implement scan token API ......................................................................
Patch Set 6: (16 comments) http://gerrit.cloudera.org:8080/#/c/2757/6/src/kudu/client/client.cc File src/kudu/client/client.cc: Line 1187: return data_->IntoKuduScanner(scanner); > Any particular reason these functions are implemented as pass-through to th No particular reason. http://gerrit.cloudera.org:8080/#/c/2757/6/src/kudu/client/client.h File src/kudu/client/client.h: Line 1086: class KUDU_EXPORT ScanToken { > Prefix public API classes with 'Kudu'. Same for ScanTokenBuilder. Done Line 1091: // Returns a KuduScanner configured for the scan token. > Nit: add that the caller owns 'scanner', and that it is unset if !Status.ok Done Line 1104: // Deserialize a serialized ScanToken into a KuduScanner. > Nit: who owns 'scanner'? Under what conditions? Done Line 1204: // Returns the schema of the projection being scanned. : KuduSchema GetProjectionSchema() const; > Why does a builder need a getter method? Done http://gerrit.cloudera.org:8080/#/c/2757/6/src/kudu/client/scan_configuration.cc File src/kudu/client/scan_configuration.cc: Line 26: #include "kudu/client/scan_predicate.h" > Nit: why the reordering? ugh. because I use the sort command, and we apparently that's not correct. a continual annoyance. fixed. Line 27: #include "kudu/gutil/casts.h" > What's this for? Done http://gerrit.cloudera.org:8080/#/c/2757/6/src/kudu/client/scan_token-internal.cc File src/kudu/client/scan_token-internal.cc: Line 79: if (!ScanTokenPB::Feature_IsValid(feature) || feature == ScanTokenPB::Unknown) { > Like in the Java client change: what's the point of checking against Unknow Because of the protobuf unknown variant handling, as explained in the other review. Line 89: unique_ptr<KuduScanner> builder(new KuduScanner(table.get())); > Nit: 'builder' is an odd name choice given that this is a real scanner, not changed to scan_builder. Line 160: RETURN_NOT_OK(builder->Open()); > Is it weird to do this on behalf of callers? It can fail in a variety of wa I did it this way to mirror the Java API: the returned scanner is fully formed, not a builder. Line 175: PartitionPruner pruner; : pruner.Init(*table->schema().schema_, table->partition_schema(), configuration_.spec()); > Nit: move this down to the pruning loop? Done http://gerrit.cloudera.org:8080/#/c/2757/6/src/kudu/client/scan_token-internal.h File src/kudu/client/scan_token-internal.h: Line 25: #include "kudu/client/client.pb.h" > Nit: ordering Done http://gerrit.cloudera.org:8080/#/c/2757/6/src/kudu/client/scan_token-test.cc File src/kudu/client/scan_token-test.cc: Line 87: for (ScanToken* token : tokens) { > Unlike the Java client test, here you're not scanning in multiple threads. Done Line 134: // Create Session > Nit: session Done http://gerrit.cloudera.org:8080/#/c/2757/6/src/kudu/common/wire_protocol.h File src/kudu/common/wire_protocol.h: Line 103: // Convert the column predicate to protobuf. > This is just code motion, right? Yes, it's mostly reorganization. http://gerrit.cloudera.org:8080/#/c/2757/6/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: Line 1264: boost::optional<ColumnPredicate> predicate; > Is this just code deduplication? Or is something else going on that I'm not deduplication. -- To view, visit http://gerrit.cloudera.org:8080/2757 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: If28e653a9b28164741a3c6f36097f0b7c72e8162 Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
