Sameer Abhyankar has posted comments on this change.

Change subject: KUDU-1363: Add in-list predicates for extracting a set of 

Patch Set 6:


Thanks for the review Adar. All the additional changes you had identified are 
now incorporated in the latest PS. I have also changed the ownership semantics 
of the list to now be owned by PredicateData. This definitely helped cleanup 
the patch and remove all the AutoReleasePoolPlumbing I had to do earlier.
File src/kudu/client/client.h:

PS5, Line 468: t until it i
> Nit: since this is a list, I think it's safe to assume plural here and just

PS5, Line 468: :AddConjunctPredicate().
             :   // The r
> Nit: singular/plural confusion here? "its value" in a discussion about mult
File src/kudu/client/

Line 303:       vector<KuduValue*> vals;
> With the new swap-based approach to taking value ownership, there's no need
File src/kudu/client/scan_predicate-internal.h:

Line 95: class InListPredicateData : public KuduPredicate::Data {
> Nit: InListPredicateData : public KuduPredicate::Data {

PS5, Line 105: std::vector<KuduValue*> val_cloned(vals_.begin(), vals_.end());
             :     return new InListPredicateData(col_, &val_cloned);
> Why do you need to do this on the heap? Can't you just do:
File src/kudu/client/

Line 127:                                                        
> Nit: we know what the size of the vector should be up-front and can pass th
No longer needed as I no longer need to allocate on the heap

Line 129:     vals_list.push_back(val_void);
> Nit: it's not really an index, it's an actual value, isn't it?
File src/kudu/common/

PS5, Line 70:                                         vector<void*>* values) {
            :   CHECK(values != nullptr);
            :   ColumnPredicate pred(PredicateType::InList, move(column), 
            :   pred.Simplify();
            :   return pred;
            : }
> Got some more tabs here. Could you double check the whole patch?

Line 160:     case PredicateType::None: return;
> Nit: could you restore the empty line that separated these two methods?
File src/kudu/common/column_predicate.h:

Line 226:   std::vector<void*> values_;
> I see what you're saying, but ownership of the vector isn't really what's i
You are right..and this did save me a lot of AutoReleasePool plumbing! Thx
File src/kudu/common/column_predicate.h:

Line 33: 
> Nit: could you revert this empty line?

Line 195:   // Merge another predicate into this Range predicate.
> Nit: could you restore the empty line that separated these two methods?
File src/kudu/common/

Line 173:         memcpy(row->mutable_cell_ptr(*col_idx_it), 
*(predicate->raw_values()->rbegin()), size);
> Nit: , size
File src/kudu/common/types.h:

Line 33: #include "kudu/util/slice.h"
> Nit: should come before slice.h.

Line 123:   const TypeInfo* t = GetTypeInfo(Type);
> Can this be rewritten as:

Line 124:   for (auto val_lkp : *values_lkp) {
> Can we move GetTypeInfo(Type) out of the loop?

Line 136:   const TypeInfo* t = GetTypeInfo(Type);
> Let's move GetTypeInfo() out of the loop.
File src/kudu/common/

Line 31: #include "kudu/util/test_util.h"
> Nit: sort alphabetically.
File src/kudu/common/

Line 333:                                  const vector<void*>* bound_src,
> As below, can you pick a different name to avoid overloads?

Line 388:                                      const RepeatedPtrField<string>& 
> This is in an anonymous namespace; it doesn't need to be declared static.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I986cb13097f1cf4af2b752f58c4c38f412a6a598
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sameer Abhyankar <>
Gerrit-Reviewer: Adar Dembo <>
Gerrit-Reviewer: Dan Burkert <>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sameer Abhyankar <>
Gerrit-Reviewer: Todd Lipcon <>
Gerrit-HasComments: Yes

Reply via email to