Sameer Abhyankar has posted comments on this change. Change subject: KUDU-1363: Add in-list predicates for extracting a set of equalities. ......................................................................
Patch Set 5: (28 comments) 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? http://gerrit.cloudera.org:8080/#/c/2986/4/src/kudu/client/client.cc File src/kudu/client/client.cc: Line 602: > Nit: const Slice& col_name. Check your new code elsewhere to make sure any Done 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. http://gerrit.cloudera.org:8080/#/c/2986/4/src/kudu/client/client.h File src/kudu/client/client.h: Line 443: // on this table. > Nit: is "inlist" useful terminology? Is it a term people from relational da Done Line 446: // the predicate is to be applied. For example, if the given column is > Nit: "The type of 'values' list" is weird. Can you reword? Maybe "the types Done PS4, Line 452: // The returned predicate takes ownership of 'value'. : // > Update this. Done Line 459: KuduValue* value); > Shouldn't need an op, the "comparison" is implicit (IN). Done Line 460: > Taking ownership of the vector by pointer itself is a little awkward becaus Done http://gerrit.cloudera.org:8080/#/c/2986/4/src/kudu/client/predicate-test.cc File src/kudu/client/predicate-test.cc: Line 288: [&] (T value) { return value >= v && value <= 0; }); > Nit: "value IN (<int-list>)" Done Line 289: ASSERT_EQ(count, CountRows(table, { > Nit: declare count as close to where it's actually used as possible. Done Line 290: table->NewComparisonPredicate("value", > You should make a copy of test_values first, so that if someone adds a test Done Line 292: KuduValue::FromInt(v)), > Nit: for (T v : test_values) { Done 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. Done Line 393: RemoveDuplicates<string>(&deduped_test_values); > t should be const-ref. Done http://gerrit.cloudera.org:8080/#/c/2986/4/src/kudu/client/scan_predicate-internal.h File src/kudu/client/scan_predicate-internal.h: PS4, Line 106: vals_.end()); : return new InListPredicateData(col_, val_cloned); : } : > This can be combined: Done Line 117: } // namespace client > Nit: add an empty line after this. Done http://gerrit.cloudera.org:8080/#/c/2986/4/src/kudu/client/scan_predicate.cc File src/kudu/client/scan_predicate.cc: Line 105: : col_(move(col)) { > Nit: extra space between values and ) Done Line 134: vals_list->push_back(val_void); > What's this comment for? Done 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. http://gerrit.cloudera.org:8080/#/c/2986/4/src/kudu/client/scan_predicate.h 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 Done http://gerrit.cloudera.org:8080/#/c/2986/4/src/kudu/client/value.cc File src/kudu/client/value.cc: Line 208: LOG(FATAL); > I think a LOG(FATAL) here would be more appropriate; if someone adds a new Done http://gerrit.cloudera.org:8080/#/c/2986/4/src/kudu/client/value.h 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 Done http://gerrit.cloudera.org:8080/#/c/2986/4/src/kudu/common/column_predicate.h File src/kudu/common/column_predicate.h: Line 169: return values_; > Nit: got a stray tab char here. Done Line 177: ColumnPredicate(PredicateType predicate_type, > Nit: explicit is only needed for single-arg constructors. Done Line 182: // Creates a new InList column predicate. > Nit: modify the two comments to make it clear what the difference in constr Done 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 http://gerrit.cloudera.org:8080/2986 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I986cb13097f1cf4af2b752f58c4c38f412a6a598 Gerrit-PatchSet: 5 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
