Dan Burkert has posted comments on this change. Change subject: Integrate ColumnPredicate into client and server ......................................................................
Patch Set 3: (16 comments) http://gerrit.cloudera.org:8080/#/c/2138/3/src/kudu/client/client.cc File src/kudu/client/client.cc: Line 1050: // Find the first tablet. > comment doesn't seem quite accurate Done Line 1054: VLOG(1) << "Short circuting scan " << ToString(); > typo Done Line 1114: if (data_->short_circuit_) { > why is this necessary? shouldn't next_req_.scanner_id() be empty in the cas Done http://gerrit.cloudera.org:8080/#/c/2138/3/src/kudu/client/scan_predicate-internal.h File src/kudu/client/scan_predicate-internal.h: Line 79: ComparisonPredicateData* Clone() const override { > it seems like there is a potential bug lurking here: AddToScanSpec std::mov This is a potential footgun, but client.h clearly indicates that AddConjunctPredicate takes ownership of the predicate, so the caller should not be using the predicate (even to call Clone) afterwords. https://github.com/cloudera/kudu/blob/master/src/kudu/client/client.h#L898 https://github.com/cloudera/kudu/blob/master/src/kudu/client/client.h#L424 http://gerrit.cloudera.org:8080/#/c/2138/3/src/kudu/client/scan_predicate.cc File src/kudu/client/scan_predicate.cc: Line 73: auto pred = ColumnPredicate::InclusiveRange(move(col_), nullptr, val_void, arena); > can you expand out this auto type? wasn't obvious from context to me what i Done Line 74: if (pred) { > I think it's worth a note explaining that InclusiveRange will return none i That's documented on the InclusiveRange method itself. http://gerrit.cloudera.org:8080/#/c/2138/3/src/kudu/client/scanner-internal.cc File src/kudu/client/scanner-internal.cc: Line 64: pool_(), > no need for these empty initializers Done Line 100: void ColumnPredicateIntoPB(const ColumnPredicate& predicate, > style: add space above Done http://gerrit.cloudera.org:8080/#/c/2138/3/src/kudu/common/generic_iterators.h File src/kudu/common/generic_iterators.h: Line 170: std::vector<std::tuple<int32_t, ColumnPredicate>> predicates_; > maybe colidx_to_predicate_ or something? Done Line 173: std::vector<size_t> non_predicates_; > I think better to rename this to non_predicate_col_indexes or somesuch -- w Done http://gerrit.cloudera.org:8080/#/c/2138/3/src/kudu/common/scan_spec.cc File src/kudu/common/scan_spec.cc: Line 57: bool ScanSpec::CanShortCircuit() { > hrm, possible thought: would it make more sense to have the 'Optimize()' st I don't think it would make much of a difference in runtime performance, and would make the class a little harder to follow. Line 106: preds.push_back(predicate.second.ToString()); > nit: indent Done Line 116: // Don't bother if we can already short circuit the scan. This also let's us > typo: lets Done http://gerrit.cloudera.org:8080/#/c/2138/3/src/kudu/tserver/tserver.proto File src/kudu/tserver/tserver.proto: Line 213: optional bytes upper = 2; > let's name this exclusive_upper or excl_upper, or even 'xupper' if you want I've been pretty consistent in naming it 'upper' throughout the codebase as opposed to 'exclusive_upper'. Range predicates were the last remaining instance of having an inclusive upper bound, so I would prefer to make a clean break, and from now on it can be assumed that upper bounds are always exclusive, as we assume that lower bounds are inclusive. Accordingly I've renamed the upper in the range predicate to 'inclusive_upper'. Let me know if you don't think this is enough. Line 243: // DEPRECATED: use column_predicates field. > in other places, we've renamed the field to DEPRECATED_foo -- that way we m Done Line 249: repeated ColumnPredicatePB column_predicates = 13; > This protocol change will allow old clients to continue to talk to new serv In this particular case we could add a little bit of logic to the client to send the predicates in both forms, but I don't think we necessarily should. -- To view, visit http://gerrit.cloudera.org:8080/2138 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ife6852680b7f59fddee688e5702c1a70944f7622 Gerrit-PatchSet: 3 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: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
