Adar Dembo has posted comments on this change. Change subject: KUDU-1363: Add in-list predicates for extracting a set of equalities. ......................................................................
Patch Set 6: (5 comments) Did you see my high-level comment about std::unordered_set vs. std::vector? We should discuss that. The rest looks good to me modulo a few nits. http://gerrit.cloudera.org:8080/#/c/2986/6/src/kudu/client/scan_predicate-internal.h File src/kudu/client/scan_predicate-internal.h: Line 27: #include "kudu/util/auto_release_pool.h" Don't need this anymore. 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(), > No longer needed as I no longer need to allocate on the heap Yeah but bear in mind that std::vector does heap allocation internally to provide storage for elements. So if you can identify the size up-front and pass that into the constructor, you can save several reallocations in the calls to push_back() below. http://gerrit.cloudera.org:8080/#/c/2986/6/src/kudu/common/column_predicate-test.cc File src/kudu/common/column_predicate-test.cc: Line 519: TEST_F(TestColumnPredicate,TestInList) { Nit: separate TestInList from the comma after TestColumnPredicate with a space. http://gerrit.cloudera.org:8080/#/c/2986/6/src/kudu/common/types.h File src/kudu/common/types.h: Line 32: #include "kudu/util/memory/arena.h" Is this still necessary? http://gerrit.cloudera.org:8080/#/c/2986/6/src/kudu/common/wire_protocol-test.cc File src/kudu/common/wire_protocol-test.cc: Line 27: #include "kudu/util/auto_release_pool.h" Don't need this? -- 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
