Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16373 )
Change subject: IMPALA-4065 Inline comparator calls into TopN::InsertBatch() ...................................................................... Patch Set 21: (8 comments) Looking pretty good. Are you planning on splitting this into two parts to finish the codegen or to update this CR? http://gerrit.cloudera.org:8080/#/c/16373/21//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16373/21//COMMIT_MSG@7 PS21, Line 7: IMPALA-4065 Inline comparator calls into TopN::InsertBatch() This isn't actually switching the codegen'd InsertBatch function to directly call the codegen'd comparator implementation, though, is it? That was the intent of the JIRA. I see that this was doing some inlining but we do want to make sure that it's a direct call to the codegen'd comparison function before we consider this finished. http://gerrit.cloudera.org:8080/#/c/16373/21//COMMIT_MSG@20 PS21, Line 20: 2. Ran ad-hoc performance test against tpcds.store_sales (2880404 What were the test queries? Can you include an example? http://gerrit.cloudera.org:8080/#/c/16373/21/be/src/util/comparator.h File be/src/util/comparator.h: PS21: I don't think we need a common base class for the comparators cause they're template arguments - it'll call Compare(). I checked out the code and tried removing the inheritance from Comparator and it compiled fine. I think we should just document the contract for Comparator types (i.e. that they need to have Less() method that takes two arguments of type const T&) in PriorityQueue. edit: I think you have already documented it over there, but I'll leave this here for posterity. http://gerrit.cloudera.org:8080/#/c/16373/21/be/src/util/comparator.h@32 PS21, Line 32: bool ALWAYS_INLINE Less(const T& lhs, const T& rhs) const { I don't think the Compare() methods on the other classes are actually overriding this case the types won't match for a template vs non-template function. http://gerrit.cloudera.org:8080/#/c/16373/21/be/src/util/priority-queue.h File be/src/util/priority-queue.h: http://gerrit.cloudera.org:8080/#/c/16373/21/be/src/util/priority-queue.h@69 PS21, Line 69: largar larger http://gerrit.cloudera.org:8080/#/c/16373/21/be/src/util/priority-queue.h@124 PS21, Line 124: - nit: add spaces around operator. Can be worth using clang-tidy to catch some of this stuff automatically - https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=65868536 http://gerrit.cloudera.org:8080/#/c/16373/21/be/src/util/priority-queue.h@128 PS21, Line 128: // elements. This is an O(log n) operation. Must not be called when the queue is empty. http://gerrit.cloudera.org:8080/#/c/16373/21/be/src/util/priority-queue.h@139 PS21, Line 139: std::move(t I don't think you need this move() because RVO kicks in for local variables - actually I think clang-tidy will complain about it. -- To view, visit http://gerrit.cloudera.org:8080/16373 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I676b4c05cf10a6946c05e317b0002c1e29e78aa8 Gerrit-Change-Number: 16373 Gerrit-PatchSet: 21 Gerrit-Owner: Qifan Chen <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Qifan Chen <[email protected]> Gerrit-Reviewer: Sahil Takiar <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Wed, 09 Sep 2020 04:46:59 +0000 Gerrit-HasComments: Yes
