Sameer Abhyankar has posted comments on this change.

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


Patch Set 6:

(20 comments)

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.

http://gerrit.cloudera.org:8080/#/c/2986/5/src/kudu/client/client.h
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
Done


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


http://gerrit.cloudera.org:8080/#/c/2986/5/src/kudu/client/predicate-test.cc
File src/kudu/client/predicate-test.cc:

Line 303:       vector<KuduValue*> vals;
> With the new swap-based approach to taking value ownership, there's no need
Done


http://gerrit.cloudera.org:8080/#/c/2986/5/src/kudu/client/scan_predicate-internal.h
File src/kudu/client/scan_predicate-internal.h:

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


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:
Done


http://gerrit.cloudera.org:8080/#/c/2986/5/src/kudu/client/scan_predicate.cc
File src/kudu/client/scan_predicate.cc:

Line 127:                                                        
col_.type_info()->physical_type(),
> 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?
Done


http://gerrit.cloudera.org:8080/#/c/2986/5/src/kudu/common/column_predicate.cc
File src/kudu/common/column_predicate.cc:

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


Line 160:     case PredicateType::None: return;
> Nit: could you restore the empty line that separated these two methods?
Done


http://gerrit.cloudera.org:8080/#/c/2986/4/src/kudu/common/column_predicate.h
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


http://gerrit.cloudera.org:8080/#/c/2986/5/src/kudu/common/column_predicate.h
File src/kudu/common/column_predicate.h:

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


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


http://gerrit.cloudera.org:8080/#/c/2986/5/src/kudu/common/key_util.cc
File src/kudu/common/key_util.cc:

Line 173:         memcpy(row->mutable_cell_ptr(*col_idx_it), 
*(predicate->raw_values()->rbegin()), size);
> Nit: , size
Done


http://gerrit.cloudera.org:8080/#/c/2986/5/src/kudu/common/types.h
File src/kudu/common/types.h:

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


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


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


Line 136:   const TypeInfo* t = GetTypeInfo(Type);
> Let's move GetTypeInfo() out of the loop.
Done


http://gerrit.cloudera.org:8080/#/c/2986/5/src/kudu/common/wire_protocol-test.cc
File src/kudu/common/wire_protocol-test.cc:

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


http://gerrit.cloudera.org:8080/#/c/2986/5/src/kudu/common/wire_protocol.cc
File src/kudu/common/wire_protocol.cc:

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


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


-- 
To view, visit http://gerrit.cloudera.org:8080/2986
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I986cb13097f1cf4af2b752f58c4c38f412a6a598
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sameer Abhyankar <abhyan...@gmail.com>
Gerrit-Reviewer: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <d...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sameer Abhyankar <abhyan...@gmail.com>
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-HasComments: Yes

Reply via email to