You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Qifan Chen (Code Review)" <ge...@cloudera.org> on 2020/09/02 02:28:40 UTC
[Impala-ASF-CR] IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
Qifan Chen has uploaded this change for review. ( http://gerrit.cloudera.org:8080/16373
Change subject: IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
......................................................................
IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
This work addresses the current limitation in TopN node by replaing the
std::priority_queue with an in-house implementation. In this way, it is
feasible to call more efficient version of comparator when comparing tuples
during the selection of top n values. The in-house version of the priority
queue is LLVM code-gened.
Testing:
1. Added a new test TestBasic in a new test harness priority-queue-test.cc to
verify that the priority queue works properly.
2. Ran Core tests successfully.
Change-Id: I676b4c05cf10a6946c05e317b0002c1e29e78aa8
---
M be/src/exec/topn-node-ir.cc
M be/src/exec/topn-node.cc
M be/src/exec/topn-node.h
M be/src/util/CMakeLists.txt
A be/src/util/comparator-wrapper.h
A be/src/util/priority-queue-test.cc
A be/src/util/priority-queue.h
M be/src/util/tuple-row-compare.cc
M be/src/util/tuple-row-compare.h
9 files changed, 387 insertions(+), 65 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/73/16373/9
--
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: newchange
Gerrit-Change-Id: I676b4c05cf10a6946c05e317b0002c1e29e78aa8
Gerrit-Change-Number: 16373
Gerrit-PatchSet: 9
Gerrit-Owner: Qifan Chen <qc...@cloudera.com>
[Impala-ASF-CR] IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/16373 )
Change subject: IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
......................................................................
IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
This work addresses the current limitation in TopN node by replacing
std::priority_queue with an in-house priority queue implementation.
In this way, call-site replacement of calls to a more efficient
version of tuple comparator becomes possible. The tuple comparator is
optimized by removing the condition test on whether the code-gen
version is ready or not.
Testing:
1. Added a new test TestBasic in a new test harness
priority-queue-test.cc to verify that the priority queue works
properly;
2. Ran ad-hoc performance test against tpcds.store_sales (2880404
rows) and saw 16% and 24% top-100 performance improvement on integer
and decimal columns respectively. The test queries are as follows.
-- SQL integer column version
select ss_sold_time_sk, ss_item_sk, ss_customer_sk
from tpcds.store_sales
order by ss_sold_time_sk, ss_item_sk
limit 100;
-- SQL decimal column version
select ss_ext_wholesale_cost, ss_ext_list_price, ss_ext_tax
from tpcds.store_sales
order by ss_ext_wholesale_cost, ss_ext_list_price
limit 100;
3. Ran Core tests successfully.
Change-Id: I676b4c05cf10a6946c05e317b0002c1e29e78aa8
Reviewed-on: http://gerrit.cloudera.org:8080/16373
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/exec/topn-node-ir.cc
M be/src/exec/topn-node.cc
M be/src/exec/topn-node.h
M be/src/util/CMakeLists.txt
A be/src/util/priority-queue-test.cc
A be/src/util/priority-queue.h
M be/src/util/tuple-row-compare.cc
M be/src/util/tuple-row-compare.h
8 files changed, 402 insertions(+), 94 deletions(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
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: merged
Gerrit-Change-Id: I676b4c05cf10a6946c05e317b0002c1e29e78aa8
Gerrit-Change-Number: 16373
Gerrit-PatchSet: 27
Gerrit-Owner: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
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 25: Code-Review+2
--
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: 25
Gerrit-Owner: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 11 Sep 2020 16:32:52 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
Posted by "Qifan Chen (Code Review)" <ge...@cloudera.org>.
Qifan Chen has uploaded a new patch set (#23). ( http://gerrit.cloudera.org:8080/16373 )
Change subject: IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
......................................................................
IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
This work addresses the current limitation in TopN node by replacing
std::priority_queue with an in-house priority queue implementation.
In this way, call-cite replacement of calls to a more efficient
version of tuple comparator becomes possible. The tuple comparator is
optimized by removing the condition test on whether the code-gen
version is ready or not.
Testing:
1. Added a new test TestBasic in a new test harness
priority-queue-test.cc to verify that the priority queue works
properly;
2. Ran ad-hoc performance test against tpcds.store_sales (2880404
rows) and saw 16% and 24% top-100 performance improvement on integer
and decimal columns respectively. The test queries are as follows.
-- SQL integer column version
select ss_sold_time_sk, ss_item_sk, ss_customer_sk
from tpcds.store_sales
order by ss_sold_time_sk, ss_item_sk
limit 100;
-- SQL decimal column version
select ss_ext_wholesale_cost, ss_ext_list_price, ss_ext_tax
from tpcds.store_sales
order by ss_ext_wholesale_cost, ss_ext_list_price
limit 100;
3. Ran Core tests successfully.
Change-Id: I676b4c05cf10a6946c05e317b0002c1e29e78aa8
---
M be/src/exec/topn-node-ir.cc
M be/src/exec/topn-node.cc
M be/src/exec/topn-node.h
M be/src/util/CMakeLists.txt
A be/src/util/priority-queue-test.cc
A be/src/util/priority-queue.h
M be/src/util/tuple-row-compare.cc
M be/src/util/tuple-row-compare.h
8 files changed, 414 insertions(+), 92 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/73/16373/23
--
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: newpatchset
Gerrit-Change-Id: I676b4c05cf10a6946c05e317b0002c1e29e78aa8
Gerrit-Change-Number: 16373
Gerrit-PatchSet: 23
Gerrit-Owner: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16373 )
Change subject: IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
......................................................................
Patch Set 19:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/7130/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 19
Gerrit-Owner: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 08 Sep 2020 18:11:21 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/7132/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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 <qc...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 08 Sep 2020 18:24:42 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] WIP IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16373 )
Change subject: WIP IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
......................................................................
Patch Set 14:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/7119/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 14
Gerrit-Owner: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Sat, 05 Sep 2020 22:30:48 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] WIP IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16373 )
Change subject: WIP IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
......................................................................
Patch Set 11:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/7111/ : Initial code review checks failed. See linked job for details on the failure.
--
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: 11
Gerrit-Owner: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 04 Sep 2020 20:26:59 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16373 )
Change subject: IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
......................................................................
Patch Set 25:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/7162/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 25
Gerrit-Owner: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 11 Sep 2020 14:03:56 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
Posted by "Qifan Chen (Code Review)" <ge...@cloudera.org>.
Qifan Chen has uploaded a new patch set (#21). ( http://gerrit.cloudera.org:8080/16373 )
Change subject: IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
......................................................................
IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
This work addresses the current limitation in TopN node by replacing
std::priority_queue with an in-house implementation. In this way, it
is feasible to inline and call more efficient version of comparator
when comparing tuples during the selection of top n values. The
in-house version of the priority queue implmentation is inlined and
LLVM code-gened.
Testing:
1. Added a new test TestBasic in a new test harness
priority-queue-test.cc to verify that the priority queue works
properly;
2. Ran ad-hoc performance test against tpcds.store_sales (2880404
rows) and saw 7% and 11% topn performance improvement with
heapifying operations on decimal and integer columns respectively.
3. Ran Core tests successfully.
Change-Id: I676b4c05cf10a6946c05e317b0002c1e29e78aa8
---
M be/src/exec/topn-node-ir.cc
M be/src/exec/topn-node.cc
M be/src/exec/topn-node.h
M be/src/util/CMakeLists.txt
A be/src/util/comparator.h
A be/src/util/priority-queue-test.cc
A be/src/util/priority-queue.h
M be/src/util/tuple-row-compare.h
8 files changed, 370 insertions(+), 68 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/73/16373/21
--
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: newpatchset
Gerrit-Change-Id: I676b4c05cf10a6946c05e317b0002c1e29e78aa8
Gerrit-Change-Number: 16373
Gerrit-PatchSet: 21
Gerrit-Owner: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16373 )
Change subject: IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
......................................................................
Patch Set 22:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/7152/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 22
Gerrit-Owner: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 10 Sep 2020 17:11:37 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] WIP IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
Posted by "Qifan Chen (Code Review)" <ge...@cloudera.org>.
Qifan Chen has uploaded a new patch set (#12). ( http://gerrit.cloudera.org:8080/16373 )
Change subject: WIP IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
......................................................................
WIP IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
This work addresses the current limitation in TopN node by replaing the
std::priority_queue with an in-house implementation. In this way, it is
feasible to call more efficient version of comparator when comparing tuples
during the selection of top n values. The in-house version of the priority
queue is LLVM code-gened.
Testing:
1. Added a new test TestBasic in a new test harness priority-queue-test.cc to
verify that the priority queue works properly.
2. Ran Core tests successfully.
TODO:
1. llvm code gen for entire TopNNode::InsertBatch();
2. Performance testing.
Change-Id: I676b4c05cf10a6946c05e317b0002c1e29e78aa8
---
M be/src/exec/topn-node-ir.cc
M be/src/exec/topn-node.cc
M be/src/exec/topn-node.h
M be/src/util/CMakeLists.txt
A be/src/util/comparator.h
A be/src/util/priority-queue-test.cc
A be/src/util/priority-queue.h
M be/src/util/tuple-row-compare.cc
M be/src/util/tuple-row-compare.h
9 files changed, 384 insertions(+), 68 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/73/16373/12
--
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: newpatchset
Gerrit-Change-Id: I676b4c05cf10a6946c05e317b0002c1e29e78aa8
Gerrit-Change-Number: 16373
Gerrit-PatchSet: 12
Gerrit-Owner: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] WIP IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
Posted by "Qifan Chen (Code Review)" <ge...@cloudera.org>.
Qifan Chen has uploaded a new patch set (#14). ( http://gerrit.cloudera.org:8080/16373 )
Change subject: WIP IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
......................................................................
WIP IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
This work addresses the current limitation in TopN node by replacing
std::priority_queue with an in-house implementation. In this way, it
is feasible to inline and call more efficient version of comparator
when comparing tuples during the selection of top n values. The
in-house version of the priority queue is LLVM code-gened.
Testing:
1. Added a new test TestBasic in a new test harness priority-queue-test.cc to
verify that the priority queue works properly.
2. Ran Core tests successfully.
TODO:
1. llvm code gen for entire TopNNode::InsertBatch();
2. Performance testing.
Change-Id: I676b4c05cf10a6946c05e317b0002c1e29e78aa8
---
M be/src/exec/topn-node-ir.cc
M be/src/exec/topn-node.cc
M be/src/exec/topn-node.h
M be/src/util/CMakeLists.txt
A be/src/util/comparator.h
A be/src/util/priority-queue-test.cc
A be/src/util/priority-queue.h
M be/src/util/tuple-row-compare.cc
M be/src/util/tuple-row-compare.h
9 files changed, 368 insertions(+), 68 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/73/16373/14
--
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: newpatchset
Gerrit-Change-Id: I676b4c05cf10a6946c05e317b0002c1e29e78aa8
Gerrit-Change-Number: 16373
Gerrit-PatchSet: 14
Gerrit-Owner: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
Posted by "Qifan Chen (Code Review)" <ge...@cloudera.org>.
Qifan Chen has uploaded a new patch set (#20). ( http://gerrit.cloudera.org:8080/16373 )
Change subject: IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
......................................................................
IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
This work addresses the current limitation in TopN node by replacing
std::priority_queue with an in-house implementation. In this way, it
is feasible to inline and call more efficient version of comparator
when comparing tuples during the selection of top n values. The
in-house version of the priority queue implmentation is inlined and
LLVM code-gened.
Testing:
1. Added a new test TestBasic in a new test harness
priority-queue-test.cc to verify that the priority queue works
properly;
2. Ran ad-hoc performance test against tpcds.store_sales on R&D box and
saw 7-10% topn performance improvement;
3. Ran Core tests successfully.
Change-Id: I676b4c05cf10a6946c05e317b0002c1e29e78aa8
---
M be/src/exec/topn-node-ir.cc
M be/src/exec/topn-node.cc
M be/src/exec/topn-node.h
M be/src/util/CMakeLists.txt
A be/src/util/comparator.h
A be/src/util/priority-queue-test.cc
A be/src/util/priority-queue.h
M be/src/util/tuple-row-compare.h
8 files changed, 370 insertions(+), 68 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/73/16373/20
--
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: newpatchset
Gerrit-Change-Id: I676b4c05cf10a6946c05e317b0002c1e29e78aa8
Gerrit-Change-Number: 16373
Gerrit-PatchSet: 20
Gerrit-Owner: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
Posted by "Qifan Chen (Code Review)" <ge...@cloudera.org>.
Qifan Chen has uploaded a new patch set (#19). ( http://gerrit.cloudera.org:8080/16373 )
Change subject: IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
......................................................................
IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
This work addresses the current limitation in TopN node by replacing
std::priority_queue with an in-house implementation. In this way, it
is feasible to inline and call more efficient version of comparator
when comparing tuples during the selection of top n values. The
in-house version of the priority queue implmentation is inlined and
LLVM code-gened.
Testing:
1. Added a new test TestBasic in a new test harness priority-queue-test.cc
to verify that the priority queue works properly;
2. Ran ad-hoc performance test against tpcds.store_sales on R&D box and
saw 7-10% topn performance improvement;
3. Ran Core tests successfully.
Change-Id: I676b4c05cf10a6946c05e317b0002c1e29e78aa8
---
M be/src/exec/topn-node-ir.cc
M be/src/exec/topn-node.cc
M be/src/exec/topn-node.h
M be/src/util/CMakeLists.txt
A be/src/util/comparator.h
A be/src/util/priority-queue-test.cc
A be/src/util/priority-queue.h
M be/src/util/tuple-row-compare.h
8 files changed, 370 insertions(+), 68 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/73/16373/19
--
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: newpatchset
Gerrit-Change-Id: I676b4c05cf10a6946c05e317b0002c1e29e78aa8
Gerrit-Change-Number: 16373
Gerrit-PatchSet: 19
Gerrit-Owner: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16373 )
Change subject: IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
......................................................................
Patch Set 20:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/7131/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 20
Gerrit-Owner: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 08 Sep 2020 18:11:57 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16373 )
Change subject: IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
......................................................................
Patch Set 26: Code-Review+2
--
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: 26
Gerrit-Owner: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 11 Sep 2020 16:33:04 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16373 )
Change subject: IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
......................................................................
Patch Set 26:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/6426/ DRY_RUN=false
--
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: 26
Gerrit-Owner: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 11 Sep 2020 16:33:05 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
Posted by "Qifan Chen (Code Review)" <ge...@cloudera.org>.
Qifan Chen has uploaded a new patch set (#18). ( http://gerrit.cloudera.org:8080/16373 )
Change subject: IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
......................................................................
IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
This work addresses the current limitation in TopN node by replacing
std::priority_queue with an in-house implementation. In this way, it
is feasible to inline and call more efficient version of comparator
when comparing tuples during the selection of top n values. The
in-house version of the priority queue implmentation is inlined and
LLVM code-gened.
Testing:
1. Added a new test TestBasic in a new test harness priority-queue-test.cc to
verify that the priority queue works properly;
2. Ran ad-hoc performance test against tpcds.store_sales on R&D box and saw
7-10% topn performance improvement;
3. Ran Core tests successfully.
Change-Id: I676b4c05cf10a6946c05e317b0002c1e29e78aa8
---
M be/src/exec/topn-node-ir.cc
M be/src/exec/topn-node.cc
M be/src/exec/topn-node.h
M be/src/util/CMakeLists.txt
A be/src/util/comparator.h
A be/src/util/priority-queue-test.cc
A be/src/util/priority-queue.h
M be/src/util/tuple-row-compare.h
8 files changed, 370 insertions(+), 68 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/73/16373/18
--
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: newpatchset
Gerrit-Change-Id: I676b4c05cf10a6946c05e317b0002c1e29e78aa8
Gerrit-Change-Number: 16373
Gerrit-PatchSet: 18
Gerrit-Owner: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
Posted by "Qifan Chen (Code Review)" <ge...@cloudera.org>.
Qifan Chen has uploaded a new patch set (#24). ( http://gerrit.cloudera.org:8080/16373 )
Change subject: IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
......................................................................
IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
This work addresses the current limitation in TopN node by replacing
std::priority_queue with an in-house priority queue implementation.
In this way, call-cite replacement of calls to a more efficient
version of tuple comparator becomes possible. The tuple comparator is
optimized by removing the condition test on whether the code-gen
version is ready or not.
Testing:
1. Added a new test TestBasic in a new test harness
priority-queue-test.cc to verify that the priority queue works
properly;
2. Ran ad-hoc performance test against tpcds.store_sales (2880404
rows) and saw 16% and 24% top-100 performance improvement on integer
and decimal columns respectively. The test queries are as follows.
-- SQL integer column version
select ss_sold_time_sk, ss_item_sk, ss_customer_sk
from tpcds.store_sales
order by ss_sold_time_sk, ss_item_sk
limit 100;
-- SQL decimal column version
select ss_ext_wholesale_cost, ss_ext_list_price, ss_ext_tax
from tpcds.store_sales
order by ss_ext_wholesale_cost, ss_ext_list_price
limit 100;
3. Ran Core tests successfully.
Change-Id: I676b4c05cf10a6946c05e317b0002c1e29e78aa8
---
M be/src/exec/topn-node-ir.cc
M be/src/exec/topn-node.cc
M be/src/exec/topn-node.h
M be/src/util/CMakeLists.txt
A be/src/util/priority-queue-test.cc
A be/src/util/priority-queue.h
M be/src/util/tuple-row-compare.cc
M be/src/util/tuple-row-compare.h
8 files changed, 414 insertions(+), 92 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/73/16373/24
--
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: newpatchset
Gerrit-Change-Id: I676b4c05cf10a6946c05e317b0002c1e29e78aa8
Gerrit-Change-Number: 16373
Gerrit-PatchSet: 24
Gerrit-Owner: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16373 )
Change subject: IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
......................................................................
Patch Set 9:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/7070/ : Initial code review checks failed. See linked job for details on the failure.
--
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: 9
Gerrit-Owner: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 02 Sep 2020 02:43:24 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16373 )
Change subject: IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
......................................................................
Patch Set 16:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/7127/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 16
Gerrit-Owner: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 08 Sep 2020 17:46:59 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
Posted by "Qifan Chen (Code Review)" <ge...@cloudera.org>.
Qifan Chen has posted comments on this change. ( http://gerrit.cloudera.org:8080/16373 )
Change subject: IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
......................................................................
Patch Set 9:
> In the work in Version 9, the STL version of the max heap is
> replaced with our version, and the topN operator works when codeGen is
> disabled.
>
> The remaining work is to code-gen the new heap code and simplify
> TupleRowComparator::Compare() by dropping the IF test on
> codegend_compare_fn.
--
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: 9
Gerrit-Owner: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 02 Sep 2020 02:36:06 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
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 <qc...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 09 Sep 2020 04:46:59 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
Posted by "Qifan Chen (Code Review)" <ge...@cloudera.org>.
Qifan Chen has uploaded a new patch set (#22). ( http://gerrit.cloudera.org:8080/16373 )
Change subject: IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
......................................................................
IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
This work addresses the current limitation in TopN node by replacing
std::priority_queue with an in-house priority queue implementation.
In this way, call-cite replacement of calls to a more efficient
version of tuple comparator becomes possible. The tuple comparator is
optimized by removing the condition test on whether the code-gen
version is ready or not.
Testing:
1. Added a new test TestBasic in a new test harness
priority-queue-test.cc to verify that the priority queue works
properly;
2. Ran ad-hoc performance test against tpcds.store_sales (2880404
rows) and saw 16% and 24% top-100 performance improvement on integer
and decimal columns respectively. The test queries are as follows.
-- SQL integer column version
select ss_sold_time_sk, ss_item_sk, ss_customer_sk
from tpcds.store_sales
order by ss_sold_time_sk, ss_item_sk
limit 100;
-- SQL decimal column version
select ss_ext_wholesale_cost, ss_ext_list_price, ss_ext_tax
from tpcds.store_sales
order by ss_ext_wholesale_cost, ss_ext_list_price
limit 100;
3. Ran Core tests successfully.
Change-Id: I676b4c05cf10a6946c05e317b0002c1e29e78aa8
---
M be/src/exec/topn-node-ir.cc
M be/src/exec/topn-node.cc
M be/src/exec/topn-node.h
M be/src/util/CMakeLists.txt
A be/src/util/priority-queue-test.cc
A be/src/util/priority-queue.h
M be/src/util/tuple-row-compare.cc
M be/src/util/tuple-row-compare.h
8 files changed, 416 insertions(+), 92 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/73/16373/22
--
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: newpatchset
Gerrit-Change-Id: I676b4c05cf10a6946c05e317b0002c1e29e78aa8
Gerrit-Change-Number: 16373
Gerrit-PatchSet: 22
Gerrit-Owner: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
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 24:
(5 comments)
Looking good, just had a few nits!
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@124
PS21, Line 124:
> nit: add spaces around operator.
*clang-format
http://gerrit.cloudera.org:8080/#/c/16373/23/be/src/util/tuple-row-compare.h
File be/src/util/tuple-row-compare.h:
http://gerrit.cloudera.org:8080/#/c/16373/23/be/src/util/tuple-row-compare.h@51
PS23, Line 51: llvm::Function*& compare_fn
nit: we usually avoid references for output arguments (cause it makes the fact that it's potentially mutated at the callsite less obvious), so this should be a **
http://gerrit.cloudera.org:8080/#/c/16373/23/be/src/util/tuple-row-compare.h@56
PS23, Line 56: =
nit: formatting
http://gerrit.cloudera.org:8080/#/c/16373/23/be/src/util/tuple-row-compare.h@164
PS23, Line 164: versoin
nit: version
http://gerrit.cloudera.org:8080/#/c/16373/23/be/src/util/tuple-row-compare.cc
File be/src/util/tuple-row-compare.cc:
http://gerrit.cloudera.org:8080/#/c/16373/23/be/src/util/tuple-row-compare.cc@83
PS23, Line 83: //llvm::Function* fn;
Remove this?
--
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: 24
Gerrit-Owner: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 10 Sep 2020 21:23:05 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16373 )
Change subject: IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
......................................................................
Patch Set 24:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/7154/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 24
Gerrit-Owner: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 10 Sep 2020 21:21:41 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] WIP IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
Posted by "Qifan Chen (Code Review)" <ge...@cloudera.org>.
Qifan Chen has uploaded a new patch set (#13). ( http://gerrit.cloudera.org:8080/16373 )
Change subject: WIP IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
......................................................................
WIP IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
This work addresses the current limitation in TopN node by replacing
std::priority_queue with an in-house implementation. In this way, it
is feasible to inline and call more efficient version of comparator
when comparing tuples during the selection of top n values. The
in-house version of the priority queue is LLVM code-gened.
Testing:
1. Added a new test TestBasic in a new test harness priority-queue-test.cc to
verify that the priority queue works properly.
2. Ran Core tests successfully.
TODO:
1. llvm code gen for entire TopNNode::InsertBatch();
2. Performance testing.
Change-Id: I676b4c05cf10a6946c05e317b0002c1e29e78aa8
---
M be/src/exec/topn-node-ir.cc
M be/src/exec/topn-node.cc
M be/src/exec/topn-node.h
M be/src/util/CMakeLists.txt
A be/src/util/comparator.h
A be/src/util/priority-queue-test.cc
A be/src/util/priority-queue.h
M be/src/util/tuple-row-compare.cc
M be/src/util/tuple-row-compare.h
9 files changed, 384 insertions(+), 68 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/73/16373/13
--
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: newpatchset
Gerrit-Change-Id: I676b4c05cf10a6946c05e317b0002c1e29e78aa8
Gerrit-Change-Number: 16373
Gerrit-PatchSet: 13
Gerrit-Owner: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] WIP IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
Posted by "Qifan Chen (Code Review)" <ge...@cloudera.org>.
Qifan Chen has posted comments on this change. ( http://gerrit.cloudera.org:8080/16373 )
Change subject: WIP IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
......................................................................
Patch Set 12:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16373/9/be/src/util/priority-queue.h
File be/src/util/priority-queue.h:
http://gerrit.cloudera.org:8080/#/c/16373/9/be/src/util/priority-queue.h@81
PS9, Line 81: public:
> Can't we pass in TupleRowComparator directly? ComparatorWrapper was just us
Done
--
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: 12
Gerrit-Owner: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Sat, 05 Sep 2020 01:57:24 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16373 )
Change subject: IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
......................................................................
Patch Set 18:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/7129/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 18
Gerrit-Owner: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 08 Sep 2020 18:12:13 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] WIP IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16373 )
Change subject: WIP IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
......................................................................
Patch Set 12:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/7115/ : Initial code review checks failed. See linked job for details on the failure.
--
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: 12
Gerrit-Owner: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Sat, 05 Sep 2020 02:12:05 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
Posted by "Qifan Chen (Code Review)" <ge...@cloudera.org>.
Qifan Chen has posted comments on this change. ( http://gerrit.cloudera.org:8080/16373 )
Change subject: IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
......................................................................
Patch Set 9:
In the work in Version 9, the STL version of the max heap is replaced with our version, and the code works when codeGen is disabled.
The remaining work is to code-gen the new heap code and simplify TupleRowComparator::Compare() by dropping the IF test on codegend_compare_fn.
--
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: 9
Gerrit-Owner: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 02 Sep 2020 02:35:26 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
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 9:
(24 comments)
Had a bunch of style comments, mostly driven by our style guide and existing best practices.
I expected to see some changes to the codegen logic or perf numbers to actually replace the indirect comparator calls in the LLVM IR with a direct call to the codegen'd function. Is there going to be a part 2 that does that?
http://gerrit.cloudera.org:8080/#/c/16373/9//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/16373/9//COMMIT_MSG@19
PS9, Line 19:
Did you do any performance comparison? Would be nice to have a targeted query that stresses the operator.
http://gerrit.cloudera.org:8080/#/c/16373/9/be/src/exec/topn-node-ir.cc
File be/src/exec/topn-node-ir.cc:
http://gerrit.cloudera.org:8080/#/c/16373/9/be/src/exec/topn-node-ir.cc@58
PS9, Line 58: priority_queue_.Heapify();
I'm a little confused by this - isn't heapify generally an O(n) operation?
http://gerrit.cloudera.org:8080/#/c/16373/9/be/src/exec/topn-node.h
File be/src/exec/topn-node.h:
http://gerrit.cloudera.org:8080/#/c/16373/9/be/src/exec/topn-node.h@188
PS9, Line 188: private :
nit: bad whitespace change
http://gerrit.cloudera.org:8080/#/c/16373/9/be/src/exec/topn-node.cc
File be/src/exec/topn-node.cc:
http://gerrit.cloudera.org:8080/#/c/16373/9/be/src/exec/topn-node.cc@128
PS9, Line 128: if (codegen_status.ok()) {
I would have expected to see some changes here to replace the comparator call with a codegen'd version.
http://gerrit.cloudera.org:8080/#/c/16373/9/be/src/exec/topn-node.cc@273
PS9, Line 273: priority_queue_.Pop(tuple);
This interface is an improvement!
http://gerrit.cloudera.org:8080/#/c/16373/9/be/src/exec/topn-node.cc@312
PS9, Line 312: : capacity_(capacity), priority_queue_(c, capacity) {}
Not the biggest deal, but this preallocates the full array whereas previously it relied on the vector to expand. Might be a small difference in memory management for larger values of N.
http://gerrit.cloudera.org:8080/#/c/16373/9/be/src/util/CMakeLists.txt
File be/src/util/CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/16373/9/be/src/util/CMakeLists.txt@151
PS9, Line 151: priority-queue-test.cc
nit: can you preserve the alphabetic ordering of the files here?
http://gerrit.cloudera.org:8080/#/c/16373/9/be/src/util/CMakeLists.txt@221
PS9, Line 221: ADD_UNIFIED_BE_LSAN_TEST(priority-queue-test "PriorityQueueTest.*")
Could preserve order here too
http://gerrit.cloudera.org:8080/#/c/16373/9/be/src/util/comparator-wrapper.h
File be/src/util/comparator-wrapper.h:
PS9:
I think we can maybe just delete this wrapper - it was only required to adapt TupleRowComparator to the C++ STL interface, and I think we can now just call it directly.
http://gerrit.cloudera.org:8080/#/c/16373/9/be/src/util/comparator-wrapper.h@19
PS9, Line 19: #ifndef IMPALA_UTIL_COMPARATOR_WRAPPER_H_
Not a big deal, but we prefer #pragma once in new code instead of traditional include guards.
https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=65868536
http://gerrit.cloudera.org:8080/#/c/16373/9/be/src/util/priority-queue-test.cc
File be/src/util/priority-queue-test.cc:
http://gerrit.cloudera.org:8080/#/c/16373/9/be/src/util/priority-queue-test.cc@33
PS9, Line 33: testInt
nit: TestInt
http://gerrit.cloudera.org:8080/#/c/16373/9/be/src/util/priority-queue.h
File be/src/util/priority-queue.h:
http://gerrit.cloudera.org:8080/#/c/16373/9/be/src/util/priority-queue.h@19
PS9, Line 19: #define IMPALA_UTIL_PRIORITY_QUEUE_H
We prefer #pragma once
http://gerrit.cloudera.org:8080/#/c/16373/9/be/src/util/priority-queue.h@81
PS9, Line 81: PriorityQueue(ComparatorWrapper<C> c, std::size_t n = 100)
Can't we pass in TupleRowComparator directly? ComparatorWrapper was just used to adapt TupleRowComparator to the c++ stl interface but I don't think we need that here.
http://gerrit.cloudera.org:8080/#/c/16373/9/be/src/util/priority-queue.h@96
PS9, Line 96: inline T& Top() { return elements_[0]; }
nit: not a big deal, but I generally recommend omitting inline from functions defined in the class body cause it's already implied.
https://en.cppreference.com/w/cpp/language/inline "A function defined entirely inside a class/struct/union definition, whether it's a member function or a non-member friend function, is implicitly an inline function. "
http://gerrit.cloudera.org:8080/#/c/16373/9/be/src/util/priority-queue.h@105
PS9, Line 105: capacity_ *= 2;
I'd prefer to use a vector<> so that we don't need to reimplement the doubling behaviour.
http://gerrit.cloudera.org:8080/#/c/16373/9/be/src/util/priority-queue.h@122
PS9, Line 122: void Pop(T& v) {
Why not just return the value? It's just a pointer so copy vs move etc doesn't matter.
You could also change it to the following if you wanted to avoid a hypothetical copy if T was a different type.
size_--;
return move(elements_[last]);
http://gerrit.cloudera.org:8080/#/c/16373/9/be/src/util/priority-queue.h@128
PS9, Line 128: Heapify(0, last);
Isn't heapify generally an O(n) operation? Popping off a heap should be O(log n) and it was before. Am I missing something?
http://gerrit.cloudera.org:8080/#/c/16373/9/be/src/util/priority-queue.h@139
PS9, Line 139: // high_bound-1.
I would have expected a function called heapify to take an arbitrary array and turn it into a valid heap, e.g. http://www.cplusplus.com/reference/algorithm/make_heap/
It looks like this is doing something else - sifting down the element at i?
http://gerrit.cloudera.org:8080/#/c/16373/9/be/src/util/priority-queue.h@146
PS9, Line 146: DCHECK
DCHECK_LE since it will print values on failure
http://gerrit.cloudera.org:8080/#/c/16373/9/be/src/util/priority-queue.h@166
PS9, Line 166: break;
else should have braces
http://gerrit.cloudera.org:8080/#/c/16373/9/be/src/util/priority-queue.h@168
PS9, Line 168: }
nit: here and elsewhere, missing blank line between function definitions
http://gerrit.cloudera.org:8080/#/c/16373/9/be/src/util/priority-queue.h@204
PS9, Line 204: std::size_t capacity_;
We use int64_t for sizes.
https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=65868536
https://google.github.io/styleguide/cppguide.html#Integer_Types
http://gerrit.cloudera.org:8080/#/c/16373/9/be/src/util/priority-queue.h@207
PS9, Line 207: // The array holding the elements.
We prefer a unique_ptr<T[]> or vector<T> to avoid need for explicit memory management.
https://cwiki.apache.org/confluence/display/IMPALA/Resource+Management+Best+Practices+in+Impala has some context about the preferred ways to manage memory.
http://gerrit.cloudera.org:8080/#/c/16373/9/be/src/util/priority-queue.h@209
PS9, Line 209: compartor
comparator
--
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: 9
Gerrit-Owner: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 02 Sep 2020 19:11:47 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16373 )
Change subject: IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
......................................................................
Patch Set 17:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/7128/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 17
Gerrit-Owner: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 08 Sep 2020 18:09:36 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16373 )
Change subject: IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
......................................................................
Patch Set 23:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/7153/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 23
Gerrit-Owner: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 10 Sep 2020 17:25:28 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] WIP IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
Posted by "Qifan Chen (Code Review)" <ge...@cloudera.org>.
Qifan Chen has uploaded a new patch set (#11). ( http://gerrit.cloudera.org:8080/16373 )
Change subject: WIP IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
......................................................................
WIP IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
This work addresses the current limitation in TopN node by replaing the
std::priority_queue with an in-house implementation. In this way, it is
feasible to call more efficient version of comparator when comparing tuples
during the selection of top n values. The in-house version of the priority
queue is LLVM code-gened.
Testing:
1. Added a new test TestBasic in a new test harness priority-queue-test.cc to
verify that the priority queue works properly.
2. Ran Core tests successfully.
TODO:
1. Removal of ComparatorWrapper from PriorityQueue;;
2. llvm code gen for entire TopNNode::InsertBatch();
3. Performance testing.
Change-Id: I676b4c05cf10a6946c05e317b0002c1e29e78aa8
---
M be/src/exec/topn-node-ir.cc
M be/src/exec/topn-node.cc
M be/src/exec/topn-node.h
M be/src/util/CMakeLists.txt
A be/src/util/comparator-wrapper.h
A be/src/util/priority-queue-test.cc
A be/src/util/priority-queue.h
M be/src/util/tuple-row-compare.cc
M be/src/util/tuple-row-compare.h
9 files changed, 380 insertions(+), 67 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/73/16373/11
--
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: newpatchset
Gerrit-Change-Id: I676b4c05cf10a6946c05e317b0002c1e29e78aa8
Gerrit-Change-Number: 16373
Gerrit-PatchSet: 11
Gerrit-Owner: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
Posted by "Qifan Chen (Code Review)" <ge...@cloudera.org>.
Qifan Chen has uploaded a new patch set (#16). ( http://gerrit.cloudera.org:8080/16373 )
Change subject: IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
......................................................................
IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
This work addresses the current limitation in TopN node by replacing
std::priority_queue with an in-house implementation. In this way, it
is feasible to inline and call more efficient version of comparator
when comparing tuples during the selection of top n values. The
in-house version of the priority queue implmentation is inlined and
LLVM code-gened.
Testing:
1. Added a new test TestBasic in a new test harness priority-queue-test.cc to
verify that the priority queue works properly;
2. Ran ad-hoc performance test against tpcds.store_sales on R&D box and saw
7-10% topn performance improvement);
3. Ran Core tests successfully.
Change-Id: I676b4c05cf10a6946c05e317b0002c1e29e78aa8
---
M be/src/exec/topn-node-ir.cc
M be/src/exec/topn-node.cc
M be/src/exec/topn-node.h
M be/src/util/CMakeLists.txt
A be/src/util/comparator.h
A be/src/util/priority-queue-test.cc
A be/src/util/priority-queue.h
M be/src/util/tuple-row-compare.cc
M be/src/util/tuple-row-compare.h
9 files changed, 369 insertions(+), 68 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/73/16373/16
--
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: newpatchset
Gerrit-Change-Id: I676b4c05cf10a6946c05e317b0002c1e29e78aa8
Gerrit-Change-Number: 16373
Gerrit-PatchSet: 16
Gerrit-Owner: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
Posted by "Qifan Chen (Code Review)" <ge...@cloudera.org>.
Qifan Chen has uploaded a new patch set (#17). ( http://gerrit.cloudera.org:8080/16373 )
Change subject: IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
......................................................................
IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
This work addresses the current limitation in TopN node by replacing
std::priority_queue with an in-house implementation. In this way, it
is feasible to inline and call more efficient version of comparator
when comparing tuples during the selection of top n values. The
in-house version of the priority queue implmentation is inlined and
LLVM code-gened.
Testing:
1. Added a new test TestBasic in a new test harness priority-queue-test.cc to
verify that the priority queue works properly;
2. Ran ad-hoc performance test against tpcds.store_sales on R&D box and saw
7-10% topn performance improvement);
3. Ran Core tests successfully.
Change-Id: I676b4c05cf10a6946c05e317b0002c1e29e78aa8
---
M be/src/exec/topn-node-ir.cc
M be/src/exec/topn-node.cc
M be/src/exec/topn-node.h
M be/src/util/CMakeLists.txt
A be/src/util/comparator.h
A be/src/util/priority-queue-test.cc
A be/src/util/priority-queue.h
M be/src/util/tuple-row-compare.h
8 files changed, 370 insertions(+), 68 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/73/16373/17
--
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: newpatchset
Gerrit-Change-Id: I676b4c05cf10a6946c05e317b0002c1e29e78aa8
Gerrit-Change-Number: 16373
Gerrit-PatchSet: 17
Gerrit-Owner: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] WIP IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16373 )
Change subject: WIP IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
......................................................................
Patch Set 13:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/7118/ : Initial code review checks failed. See linked job for details on the failure.
--
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: 13
Gerrit-Owner: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Sat, 05 Sep 2020 15:59:19 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
Posted by "Qifan Chen (Code Review)" <ge...@cloudera.org>.
Qifan Chen has uploaded a new patch set (#25). ( http://gerrit.cloudera.org:8080/16373 )
Change subject: IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
......................................................................
IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
This work addresses the current limitation in TopN node by replacing
std::priority_queue with an in-house priority queue implementation.
In this way, call-site replacement of calls to a more efficient
version of tuple comparator becomes possible. The tuple comparator is
optimized by removing the condition test on whether the code-gen
version is ready or not.
Testing:
1. Added a new test TestBasic in a new test harness
priority-queue-test.cc to verify that the priority queue works
properly;
2. Ran ad-hoc performance test against tpcds.store_sales (2880404
rows) and saw 16% and 24% top-100 performance improvement on integer
and decimal columns respectively. The test queries are as follows.
-- SQL integer column version
select ss_sold_time_sk, ss_item_sk, ss_customer_sk
from tpcds.store_sales
order by ss_sold_time_sk, ss_item_sk
limit 100;
-- SQL decimal column version
select ss_ext_wholesale_cost, ss_ext_list_price, ss_ext_tax
from tpcds.store_sales
order by ss_ext_wholesale_cost, ss_ext_list_price
limit 100;
3. Ran Core tests successfully.
Change-Id: I676b4c05cf10a6946c05e317b0002c1e29e78aa8
---
M be/src/exec/topn-node-ir.cc
M be/src/exec/topn-node.cc
M be/src/exec/topn-node.h
M be/src/util/CMakeLists.txt
A be/src/util/priority-queue-test.cc
A be/src/util/priority-queue.h
M be/src/util/tuple-row-compare.cc
M be/src/util/tuple-row-compare.h
8 files changed, 402 insertions(+), 94 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/73/16373/25
--
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: newpatchset
Gerrit-Change-Id: I676b4c05cf10a6946c05e317b0002c1e29e78aa8
Gerrit-Change-Number: 16373
Gerrit-PatchSet: 25
Gerrit-Owner: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16373 )
Change subject: IMPALA-4065 Inline comparator calls into TopN::InsertBatch()
......................................................................
Patch Set 26: Verified+1
--
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: 26
Gerrit-Owner: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 11 Sep 2020 21:45:29 +0000
Gerrit-HasComments: No