Sameer Abhyankar has posted comments on this change.

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

Patch Set 5:


Hey folks - I believe most of the changes that Adar had identified are now 
included in the latest patchset-5. Can someone review and let me know if there 
are additional updates required?
File src/kudu/client/

Line 602: 
> Nit: const Slice& col_name. Check your new code elsewhere to make sure any 

Line 619:     return new KuduPredicate(new ErrorPredicateData(
> Why not?
I originally left this out as I felt a list of (true, false) boolean values 
dont make sense (you might as well not put in the filter). However, I thought 
about it a little more and realized, that if a column is "nullable", you might 
just want the not null values (instead of using a col is null predicate). I 
have thus added the support for Boolean columns.

Line 624: }
> Nit: Status messages should start with a lower-case letter.
Does not apply since the check is no-longer required. InList is supported for 
Bool as well.

Line 626: KuduPredicate* KuduTable::NewInListPredicate(const Slice& col_name,
> Should we also check that every type in 'values' is equal to s->column(col_
The reason this was left out was because this check is done when we do a 
CheckTypeAndGetPointer later.
File src/kudu/client/client.h:

Line 443:   // on this table.
> Nit: is "inlist" useful terminology? Is it a term people from relational da

Line 446:   // the predicate is to be applied. For example, if the given column 
> Nit: "The type of 'values' list" is weird. Can you reword? Maybe "the types

PS4, Line 452:   // The returned predicate takes ownership of 'value'.
             :   //
> Update this.

Line 459:                                         KuduValue* value);
> Shouldn't need an op, the "comparison" is implicit (IN).

Line 460: 
> Taking ownership of the vector by pointer itself is a little awkward becaus
File src/kudu/client/

Line 288:                              [&] (T value) { return value >= v && 
value <= 0; });
> Nit: "value IN (<int-list>)"

Line 289:         ASSERT_EQ(count, CountRows(table, {
> Nit: declare count as close to where it's actually used as possible.

Line 290:               table->NewComparisonPredicate("value",
> You should make a copy of test_values first, so that if someone adds a test

Line 292:                                             KuduValue::FromInt(v)),
> Nit: for (T v : test_values) {

PS4, Line 305:   vals->push_back(KuduValue::FromInt(v));
             :       }
> Why do you need this? Doesn't the KuduPredicate take ownership of vals and 
Yep - Agreed. However, I do need to delete the vals * as asan will complain 
that I have a 8byte leak. KuduPredicate will take ownership of the values (via 
"swap" that you had mentioned elsewhere)

Line 388:       }
> You're not modifying v, so you should iterate by const-ref. On L391 too.

Line 393:       RemoveDuplicates<string>(&deduped_test_values);
> t should be const-ref.
File src/kudu/client/scan_predicate-internal.h:

PS4, Line 106:                                                                  
             :     return new InListPredicateData(col_, val_cloned);
             :   }
> This can be combined:

Line 117: } // namespace client
> Nit: add an empty line after this.
File src/kudu/client/

Line 105:     : col_(move(col)) {
> Nit: extra space between values and )

Line 134:     vals_list->push_back(val_void);
> What's this comment for?

Line 140: }
> With the suggestion I gave you for how to handle the incoming values array,
I am not sure if set<KuduValue*> will necessarily dedup as distinct pointers 
could possibly be pointing to the same value no? Given this, I have left this 
part of the code as is.
File src/kudu/client/scan_predicate.h:

Line 39:   };
> You shouldn't need this; InList implies both a kind of value (a list) and a
File src/kudu/client/

Line 208:   LOG(FATAL);
> I think a LOG(FATAL) here would be more appropriate; if someone adds a new 
File src/kudu/client/value.h:

PS4, Line 53:   // Returns true if KuduValue A < KuduValue B
            :   friend bool operator<(const KuduValue& a, const KuduValue& b);
            :   // Returns true if KuduValue A == KuduValue B
            :   friend bool operator==(const KuduValue& 
> The google style guide (which Kudu uses) generally discourages operator ove
File src/kudu/common/column_predicate.h:

Line 169:     return values_;
> Nit: got a stray tab char here.

Line 177:   ColumnPredicate(PredicateType predicate_type,
> Nit: explicit is only needed for single-arg constructors.

Line 182:   // Creates a new InList column predicate.
> Nit: modify the two comments to make it clear what the difference in constr

Line 226:   std::vector<void*>* values_;
> Why does this need to be a pointer? Can't the ColumnPredicate own its own v
Currently the vector is owned by PredicateData.. sort of similar to how the 
ownership is for non list type predicates. I chose to follow the same approach 
to avoid any confusion.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I986cb13097f1cf4af2b752f58c4c38f412a6a598
Gerrit-PatchSet: 5
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