Adar Dembo has posted comments on this change. Change subject: KUDU-1363: Add in-list predicates for extracting a set of equalities. ......................................................................
Patch Set 3: (6 comments) Sameer, thanks for the patch. I've reviewed just the client-facing API pieces for now. I'll review the rest once you've addressed the memory leak that LeakSanitizer surfaced. To see the leak, click on the link in the latest "Kudu Jenkins" entry in your gerrit. Click on ASAN, then Console Output. Scroll to the bottom and click on the dist-test link. Download the artifacts for the failed tests and look inside the archives. You should see some test logs; these contain LeakSanitizer stack traces that should help you identify the memory leaks. Once you've addressed the leaks, I'll review the patch in more detail. http://gerrit.cloudera.org:8080/#/c/2986/3/src/kudu/client/value.h File src/kudu/client/value.h: Line 26: #include <vector> Nit: these should come before the KUDU_HEADERS_NO_STUBS check (see the google C++ style guide in thirdparty; system headers before project headers) and should be alphabetically sorted. Line 35: class KUDU_EXPORT KuduValue { The new methods that copy from an existing vector should take vector as const-ref input, unless there's a specific reason you're passing by value that I'm not seeing? Oh, I see, it's because some of the FromFooList() method implementations remove duplicates by modifying the input vector. Perhaps You could perform the deduplication during assignment to Data? Line 43: // Construct a vector of KuduValue* from the given list of integers. I don't suppose it's possible to have a single method for any list? template <typename T> static KuduValue* FromList(const std::vector<T>& v); I'm not great with templates myself so maybe that's not possible for some reason. Line 72: static void RemoveDupsFromList(std::vector<T> *); Why does this need to be public? Also, missing a variable name. Also, the pointer asterisk should be adjacent to the type (see the style guide). http://gerrit.cloudera.org:8080/#/c/2986/3/src/kudu/common/column_predicate.h File src/kudu/common/column_predicate.h: Line 34: using std::vector; The style guide prohibits "using" statements in headers. Line 110: static ColumnPredicate InList(ColumnSchema column, vector<void*>* values); Nit: should fully qualify (i.e. std::vector) here. -- 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: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Sameer Abhyankar <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sameer Abhyankar <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
