[Impala-ASF-CR] IMPALA-4080 [part 1]: Move codegen code from aggregation exec nodes to their plan nodes
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15053 ) Change subject: IMPALA-4080 [part 1]: Move codegen code from aggregation exec nodes to their plan nodes .. Patch Set 8: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/15053 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I58f52a262ac7d0af259d5bcda72ada93a851d3b2 Gerrit-Change-Number: 15053 Gerrit-PatchSet: 8 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 31 Jan 2020 03:06:23 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4080 [part 1]: Move codegen code from aggregation exec nodes to their plan nodes
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/15053 ) Change subject: IMPALA-4080 [part 1]: Move codegen code from aggregation exec nodes to their plan nodes .. IMPALA-4080 [part 1]: Move codegen code from aggregation exec nodes to their plan nodes Refactored code to move codegen code from aggregation exec nodes to their plan nodes. Added some TODOs that will be fixed in the next few patch. Testing: - Ran queries and confirmed manually that the codegened code works. - Ran all e2e tests for agg nodes and partition joins. Change-Id: I58f52a262ac7d0af259d5bcda72ada93a851d3b2 Reviewed-on: http://gerrit.cloudera.org:8080/15053 Reviewed-by: Csaba Ringhofer Tested-by: Impala Public Jenkins --- M be/src/exec/aggregation-node-base.cc M be/src/exec/aggregator.cc M be/src/exec/aggregator.h M be/src/exec/grouping-aggregator.cc M be/src/exec/grouping-aggregator.h M be/src/exec/hash-table.cc M be/src/exec/hash-table.h M be/src/exec/non-grouping-aggregator.cc M be/src/exec/non-grouping-aggregator.h M be/src/exec/partitioned-hash-join-builder.cc M be/src/exec/partitioned-hash-join-builder.h M be/src/exec/partitioned-hash-join-node.cc M be/src/exprs/expr-test.cc M be/src/exprs/scalar-expr.cc M be/src/exprs/scalar-expr.h 15 files changed, 448 insertions(+), 224 deletions(-) Approvals: Csaba Ringhofer: Looks good to me, approved Impala Public Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/15053 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I58f52a262ac7d0af259d5bcda72ada93a851d3b2 Gerrit-Change-Number: 15053 Gerrit-PatchSet: 9 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-4080 [part 1]: Move codegen code from aggregation exec nodes to their plan nodes
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15053 ) Change subject: IMPALA-4080 [part 1]: Move codegen code from aggregation exec nodes to their plan nodes .. Patch Set 8: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/5479/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/15053 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I58f52a262ac7d0af259d5bcda72ada93a851d3b2 Gerrit-Change-Number: 15053 Gerrit-PatchSet: 8 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 30 Jan 2020 22:13:18 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4080 [part 1]: Move codegen code from aggregation exec nodes to their plan nodes
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/15053 ) Change subject: IMPALA-4080 [part 1]: Move codegen code from aggregation exec nodes to their plan nodes .. Patch Set 8: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/15053 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I58f52a262ac7d0af259d5bcda72ada93a851d3b2 Gerrit-Change-Number: 15053 Gerrit-PatchSet: 8 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 30 Jan 2020 21:47:21 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4080 [part 1]: Move codegen code from aggregation exec nodes to their plan nodes
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15053 ) Change subject: IMPALA-4080 [part 1]: Move codegen code from aggregation exec nodes to their plan nodes .. Patch Set 8: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/5556/ : 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/15053 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I58f52a262ac7d0af259d5bcda72ada93a851d3b2 Gerrit-Change-Number: 15053 Gerrit-PatchSet: 8 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 30 Jan 2020 21:41:39 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4080 [part 1]: Move codegen code from aggregation exec nodes to their plan nodes
Hello Daniel Becker, Tim Armstrong, Csaba Ringhofer, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/15053 to look at the new patch set (#8). Change subject: IMPALA-4080 [part 1]: Move codegen code from aggregation exec nodes to their plan nodes .. IMPALA-4080 [part 1]: Move codegen code from aggregation exec nodes to their plan nodes Refactored code to move codegen code from aggregation exec nodes to their plan nodes. Added some TODOs that will be fixed in the next few patch. Testing: - Ran queries and confirmed manually that the codegened code works. - Ran all e2e tests for agg nodes and partition joins. Change-Id: I58f52a262ac7d0af259d5bcda72ada93a851d3b2 --- M be/src/exec/aggregation-node-base.cc M be/src/exec/aggregator.cc M be/src/exec/aggregator.h M be/src/exec/grouping-aggregator.cc M be/src/exec/grouping-aggregator.h M be/src/exec/hash-table.cc M be/src/exec/hash-table.h M be/src/exec/non-grouping-aggregator.cc M be/src/exec/non-grouping-aggregator.h M be/src/exec/partitioned-hash-join-builder.cc M be/src/exec/partitioned-hash-join-builder.h M be/src/exec/partitioned-hash-join-node.cc M be/src/exprs/expr-test.cc M be/src/exprs/scalar-expr.cc M be/src/exprs/scalar-expr.h 15 files changed, 448 insertions(+), 224 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/53/15053/8 -- To view, visit http://gerrit.cloudera.org:8080/15053 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I58f52a262ac7d0af259d5bcda72ada93a851d3b2 Gerrit-Change-Number: 15053 Gerrit-PatchSet: 8 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-4080 [part 1]: Move codegen code from aggregation exec nodes to their plan nodes
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15053 ) Change subject: IMPALA-4080 [part 1]: Move codegen code from aggregation exec nodes to their plan nodes .. Patch Set 7: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/5552/ : 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/15053 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I58f52a262ac7d0af259d5bcda72ada93a851d3b2 Gerrit-Change-Number: 15053 Gerrit-PatchSet: 7 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 30 Jan 2020 20:10:48 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4080 [part 1]: Move codegen code from aggregation exec nodes to their plan nodes
Bikramjeet Vig has posted comments on this change. (
http://gerrit.cloudera.org:8080/15053 )
Change subject: IMPALA-4080 [part 1]: Move codegen code from aggregation exec
nodes to their plan nodes
..
Patch Set 7:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/15053/6/be/src/exec/aggregator.h
File be/src/exec/aggregator.h:
http://gerrit.cloudera.org:8080/#/c/15053/6/be/src/exec/aggregator.h@132
PS6, Line 132: const std::string& name);
> nit: indent +2
Done
http://gerrit.cloudera.org:8080/#/c/15053/6/be/src/exec/grouping-aggregator.h
File be/src/exec/grouping-aggregator.h:
http://gerrit.cloudera.org:8080/#/c/15053/6/be/src/exec/grouping-aggregator.h@126
PS6, Line 126: ~GroupingAggregatorConfig() override {}
> please add 'override'
Done
http://gerrit.cloudera.org:8080/#/c/15053/6/be/src/exec/grouping-aggregator.h@165
PS6, Line 165:
> nit: double space
Done
http://gerrit.cloudera.org:8080/#/c/15053/6/be/src/exec/grouping-aggregator.h@176
PS6, Line 176: /// function.
> nit: +1 /
Done
http://gerrit.cloudera.org:8080/#/c/15053/6/be/src/exec/hash-table.h
File be/src/exec/hash-table.h:
http://gerrit.cloudera.org:8080/#/c/15053/6/be/src/exec/hash-table.h@51
PS6, Line 51: class
> ScalarExprsResultsRowLayout is a struct, which lead to the complaints by cl
Done
http://gerrit.cloudera.org:8080/#/c/15053/6/be/src/exec/hash-table.cc
File be/src/exec/hash-table.cc:
http://gerrit.cloudera.org:8080/#/c/15053/6/be/src/exec/hash-table.cc@951
PS6, Line 951: c
> nit: extra space
Done
http://gerrit.cloudera.org:8080/#/c/15053/6/be/src/exec/non-grouping-aggregator.h
File be/src/exec/non-grouping-aggregator.h:
http://gerrit.cloudera.org:8080/#/c/15053/6/be/src/exec/non-grouping-aggregator.h@46
PS6, Line 46: ~NonGroupingAggregatorConfig() override {}
> please add 'override'
Done
http://gerrit.cloudera.org:8080/#/c/15053/6/be/src/exec/non-grouping-aggregator.h@101
PS6, Line 101: referenc
> typo: reference
Done
http://gerrit.cloudera.org:8080/#/c/15053/6/be/src/exprs/scalar-expr.h
File be/src/exprs/scalar-expr.h:
http://gerrit.cloudera.org:8080/#/c/15053/6/be/src/exprs/scalar-expr.h@203
PS6, Line 203: static std::string DebugString(const std::vector&
exprs);
: std::string DebugString(const std::str
> optional: this could be a constructor in ScalarExprsResultsRowLayout
good idea!
--
To view, visit http://gerrit.cloudera.org:8080/15053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58f52a262ac7d0af259d5bcda72ada93a851d3b2
Gerrit-Change-Number: 15053
Gerrit-PatchSet: 7
Gerrit-Owner: Bikramjeet Vig
Gerrit-Reviewer: Bikramjeet Vig
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Daniel Becker
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong
Gerrit-Comment-Date: Thu, 30 Jan 2020 19:27:15 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4080 [part 1]: Move codegen code from aggregation exec nodes to their plan nodes
Hello Daniel Becker, Tim Armstrong, Csaba Ringhofer, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/15053 to look at the new patch set (#7). Change subject: IMPALA-4080 [part 1]: Move codegen code from aggregation exec nodes to their plan nodes .. IMPALA-4080 [part 1]: Move codegen code from aggregation exec nodes to their plan nodes Refactored code to move codegen code from aggregation exec nodes to their plan nodes. Added some TODOs that will be fixed in the next few patch. Testing: - Ran queries and confirmed manually that the codegened code works. - Ran all e2e tests for agg nodes and partition joins. Change-Id: I58f52a262ac7d0af259d5bcda72ada93a851d3b2 --- M be/src/exec/aggregation-node-base.cc M be/src/exec/aggregator.cc M be/src/exec/aggregator.h M be/src/exec/grouping-aggregator.cc M be/src/exec/grouping-aggregator.h M be/src/exec/hash-table.cc M be/src/exec/hash-table.h M be/src/exec/non-grouping-aggregator.cc M be/src/exec/non-grouping-aggregator.h M be/src/exec/partitioned-hash-join-builder.cc M be/src/exec/partitioned-hash-join-builder.h M be/src/exec/partitioned-hash-join-node.cc M be/src/exprs/expr-test.cc M be/src/exprs/scalar-expr.cc M be/src/exprs/scalar-expr.h 15 files changed, 448 insertions(+), 224 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/53/15053/7 -- To view, visit http://gerrit.cloudera.org:8080/15053 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I58f52a262ac7d0af259d5bcda72ada93a851d3b2 Gerrit-Change-Number: 15053 Gerrit-PatchSet: 7 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-4080 [part 1]: Move codegen code from aggregation exec nodes to their plan nodes
Csaba Ringhofer has posted comments on this change. (
http://gerrit.cloudera.org:8080/15053 )
Change subject: IMPALA-4080 [part 1]: Move codegen code from aggregation exec
nodes to their plan nodes
..
Patch Set 6: Code-Review+2
(10 comments)
Sorry for responding so late. I found a few nits (mainly in cut'n'pasted code)
when a I went through the code. The general direction seems good to me.
http://gerrit.cloudera.org:8080/#/c/15053/6/be/src/exec/aggregator.h
File be/src/exec/aggregator.h:
http://gerrit.cloudera.org:8080/#/c/15053/6/be/src/exec/aggregator.h@132
PS6, Line 132: const AggregatorConfig& config, const std::string& name);
nit: indent +2
http://gerrit.cloudera.org:8080/#/c/15053/6/be/src/exec/grouping-aggregator.h
File be/src/exec/grouping-aggregator.h:
http://gerrit.cloudera.org:8080/#/c/15053/6/be/src/exec/grouping-aggregator.h@126
PS6, Line 126: ~GroupingAggregatorConfig() {}
please add 'override'
http://gerrit.cloudera.org:8080/#/c/15053/6/be/src/exec/grouping-aggregator.h@165
PS6, Line 165:
nit: double space
http://gerrit.cloudera.org:8080/#/c/15053/6/be/src/exec/grouping-aggregator.h@176
PS6, Line 176: // function.
nit: +1 /
http://gerrit.cloudera.org:8080/#/c/15053/6/be/src/exec/hash-table.h
File be/src/exec/hash-table.h:
http://gerrit.cloudera.org:8080/#/c/15053/6/be/src/exec/hash-table.h@51
PS6, Line 51: class
ScalarExprsResultsRowLayout is a struct, which lead to the complaints by clang
tidy.
http://gerrit.cloudera.org:8080/#/c/15053/6/be/src/exec/hash-table.cc
File be/src/exec/hash-table.cc:
http://gerrit.cloudera.org:8080/#/c/15053/6/be/src/exec/hash-table.cc@951
PS6, Line 951:
nit: extra space
http://gerrit.cloudera.org:8080/#/c/15053/6/be/src/exec/non-grouping-aggregator.h
File be/src/exec/non-grouping-aggregator.h:
http://gerrit.cloudera.org:8080/#/c/15053/6/be/src/exec/non-grouping-aggregator.h@46
PS6, Line 46: ~NonGroupingAggregatorConfig() {}
please add 'override'
http://gerrit.cloudera.org:8080/#/c/15053/6/be/src/exec/non-grouping-aggregator.h@57
PS6, Line 57: Assumeing
typo: assuming
http://gerrit.cloudera.org:8080/#/c/15053/6/be/src/exec/non-grouping-aggregator.h@101
PS6, Line 101: refernce
typo: reference
http://gerrit.cloudera.org:8080/#/c/15053/6/be/src/exprs/scalar-expr.h
File be/src/exprs/scalar-expr.h:
http://gerrit.cloudera.org:8080/#/c/15053/6/be/src/exprs/scalar-expr.h@203
PS6, Line 203: static ScalarExprsResultsRowLayout ComputeResultsLayout(
: const vector& exprs);
optional: this could be a constructor in ScalarExprsResultsRowLayout
--
To view, visit http://gerrit.cloudera.org:8080/15053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58f52a262ac7d0af259d5bcda72ada93a851d3b2
Gerrit-Change-Number: 15053
Gerrit-PatchSet: 6
Gerrit-Owner: Bikramjeet Vig
Gerrit-Reviewer: Bikramjeet Vig
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Daniel Becker
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong
Gerrit-Comment-Date: Tue, 28 Jan 2020 14:23:07 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4080 [part 1]: Move codegen code from aggregation exec nodes to their plan nodes
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15053 ) Change subject: IMPALA-4080 [part 1]: Move codegen code from aggregation exec nodes to their plan nodes .. Patch Set 6: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/15053 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I58f52a262ac7d0af259d5bcda72ada93a851d3b2 Gerrit-Change-Number: 15053 Gerrit-PatchSet: 6 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 27 Jan 2020 19:04:45 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4080 [part 1]: Move codegen code from aggregation exec nodes to their plan nodes
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15053 ) Change subject: IMPALA-4080 [part 1]: Move codegen code from aggregation exec nodes to their plan nodes .. Patch Set 6: Build Failed https://jenkins.impala.io/job/gerrit-code-review-checks/5500/ : Initial code review checks failed. See linked job for details on the failure. -- To view, visit http://gerrit.cloudera.org:8080/15053 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I58f52a262ac7d0af259d5bcda72ada93a851d3b2 Gerrit-Change-Number: 15053 Gerrit-PatchSet: 6 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 23 Jan 2020 00:39:38 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4080 [part 1]: Move codegen code from aggregation exec nodes to their plan nodes
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/15053 ) Change subject: IMPALA-4080 [part 1]: Move codegen code from aggregation exec nodes to their plan nodes .. Patch Set 6: carried over Tim's +1 -- To view, visit http://gerrit.cloudera.org:8080/15053 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I58f52a262ac7d0af259d5bcda72ada93a851d3b2 Gerrit-Change-Number: 15053 Gerrit-PatchSet: 6 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 22 Jan 2020 23:56:12 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4080 [part 1]: Move codegen code from aggregation exec nodes to their plan nodes
Bikramjeet Vig has posted comments on this change. (
http://gerrit.cloudera.org:8080/15053 )
Change subject: IMPALA-4080 [part 1]: Move codegen code from aggregation exec
nodes to their plan nodes
..
Patch Set 6: Code-Review+1
(1 comment)
cleaned up a bit more.
http://gerrit.cloudera.org:8080/#/c/15053/5/be/src/exec/hash-table.h
File be/src/exec/hash-table.h:
http://gerrit.cloudera.org:8080/#/c/15053/5/be/src/exec/hash-table.h@114
PS5, Line 114: struct HashTableConfig {
> Can we make any more of these members const? It looks like they're mostly i
Done
--
To view, visit http://gerrit.cloudera.org:8080/15053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58f52a262ac7d0af259d5bcda72ada93a851d3b2
Gerrit-Change-Number: 15053
Gerrit-PatchSet: 6
Gerrit-Owner: Bikramjeet Vig
Gerrit-Reviewer: Bikramjeet Vig
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Daniel Becker
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong
Gerrit-Comment-Date: Wed, 22 Jan 2020 23:55:49 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4080 [part 1]: Move codegen code from aggregation exec nodes to their plan nodes
Hello Daniel Becker, Tim Armstrong, Csaba Ringhofer, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/15053 to look at the new patch set (#6). Change subject: IMPALA-4080 [part 1]: Move codegen code from aggregation exec nodes to their plan nodes .. IMPALA-4080 [part 1]: Move codegen code from aggregation exec nodes to their plan nodes Refactored code to move codegen code from aggregation exec nodes to their plan nodes. Added some TODOs that will be fixed in the next few patch. Testing: - Ran queries and confirmed manually that the codegened code works. - Ran all e2e tests for agg nodes and partition joins. Change-Id: I58f52a262ac7d0af259d5bcda72ada93a851d3b2 --- M be/src/exec/aggregation-node-base.cc M be/src/exec/aggregator.cc M be/src/exec/aggregator.h M be/src/exec/grouping-aggregator.cc M be/src/exec/grouping-aggregator.h M be/src/exec/hash-table.cc M be/src/exec/hash-table.h M be/src/exec/non-grouping-aggregator.cc M be/src/exec/non-grouping-aggregator.h M be/src/exec/partitioned-hash-join-builder.cc M be/src/exec/partitioned-hash-join-builder.h M be/src/exec/partitioned-hash-join-node.cc M be/src/exprs/expr-test.cc M be/src/exprs/scalar-expr.cc M be/src/exprs/scalar-expr.h 15 files changed, 447 insertions(+), 218 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/53/15053/6 -- To view, visit http://gerrit.cloudera.org:8080/15053 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I58f52a262ac7d0af259d5bcda72ada93a851d3b2 Gerrit-Change-Number: 15053 Gerrit-PatchSet: 6 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-4080 [part 1]: Move codegen code from aggregation exec nodes to their plan nodes
Tim Armstrong has posted comments on this change. (
http://gerrit.cloudera.org:8080/15053 )
Change subject: IMPALA-4080 [part 1]: Move codegen code from aggregation exec
nodes to their plan nodes
..
Patch Set 5: Code-Review+1
(1 comment)
LGTM aside from one thing. I can upgrade to a +2 tomorrow, but wanted to give
Csaba and Daniel another chance to look at it.
http://gerrit.cloudera.org:8080/#/c/15053/5/be/src/exec/hash-table.h
File be/src/exec/hash-table.h:
http://gerrit.cloudera.org:8080/#/c/15053/5/be/src/exec/hash-table.h@114
PS5, Line 114: struct HashTableConfig {
Can we make any more of these members const? It looks like they're mostly
initialised by the constructor
--
To view, visit http://gerrit.cloudera.org:8080/15053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58f52a262ac7d0af259d5bcda72ada93a851d3b2
Gerrit-Change-Number: 15053
Gerrit-PatchSet: 5
Gerrit-Owner: Bikramjeet Vig
Gerrit-Reviewer: Bikramjeet Vig
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Daniel Becker
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong
Gerrit-Comment-Date: Wed, 22 Jan 2020 23:24:27 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4080 [part 1]: Move codegen code from aggregation exec nodes to their plan nodes
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15053 ) Change subject: IMPALA-4080 [part 1]: Move codegen code from aggregation exec nodes to their plan nodes .. Patch Set 4: Build Failed https://jenkins.impala.io/job/gerrit-code-review-checks/5499/ : Initial code review checks failed. See linked job for details on the failure. -- To view, visit http://gerrit.cloudera.org:8080/15053 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I58f52a262ac7d0af259d5bcda72ada93a851d3b2 Gerrit-Change-Number: 15053 Gerrit-PatchSet: 4 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 22 Jan 2020 22:26:44 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4080 [part 1]: Move codegen code from aggregation exec nodes to their plan nodes
Hello Daniel Becker, Tim Armstrong, Csaba Ringhofer, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/15053 to look at the new patch set (#5). Change subject: IMPALA-4080 [part 1]: Move codegen code from aggregation exec nodes to their plan nodes .. IMPALA-4080 [part 1]: Move codegen code from aggregation exec nodes to their plan nodes Refactored code to move codegen code from aggregation exec nodes to their plan nodes. Added some TODOs that will be fixed in the next few patch. Testing: - Ran queries and confirmed manually that the codegened code works. - Ran all e2e tests for agg nodes and partition joins. Change-Id: I58f52a262ac7d0af259d5bcda72ada93a851d3b2 --- M be/src/exec/aggregation-node-base.cc M be/src/exec/aggregator.cc M be/src/exec/aggregator.h M be/src/exec/grouping-aggregator.cc M be/src/exec/grouping-aggregator.h M be/src/exec/hash-table.cc M be/src/exec/hash-table.h M be/src/exec/non-grouping-aggregator.cc M be/src/exec/non-grouping-aggregator.h M be/src/exec/partitioned-hash-join-builder.cc M be/src/exec/partitioned-hash-join-builder.h M be/src/exec/partitioned-hash-join-node.cc M be/src/exprs/expr-test.cc M be/src/exprs/scalar-expr.cc M be/src/exprs/scalar-expr.h 15 files changed, 451 insertions(+), 213 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/53/15053/5 -- To view, visit http://gerrit.cloudera.org:8080/15053 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I58f52a262ac7d0af259d5bcda72ada93a851d3b2 Gerrit-Change-Number: 15053 Gerrit-PatchSet: 5 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-4080 [part 1]: Move codegen code from aggregation exec nodes to their plan nodes
Hello Daniel Becker, Tim Armstrong, Csaba Ringhofer, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/15053 to look at the new patch set (#4). Change subject: IMPALA-4080 [part 1]: Move codegen code from aggregation exec nodes to their plan nodes .. IMPALA-4080 [part 1]: Move codegen code from aggregation exec nodes to their plan nodes Refactored code to move codegen code from aggregation exec nodes to their plan nodes. Added some TODOs that will be fixed in the next few patch. Testing: - Ran queries and confirmed manually that the codegen code works perfectly. - ran all tests for agg nodes and partition joins Change-Id: I58f52a262ac7d0af259d5bcda72ada93a851d3b2 --- M be/src/exec/aggregation-node-base.cc M be/src/exec/aggregator.cc M be/src/exec/aggregator.h M be/src/exec/grouping-aggregator.cc M be/src/exec/grouping-aggregator.h M be/src/exec/hash-table.cc M be/src/exec/hash-table.h M be/src/exec/non-grouping-aggregator.cc M be/src/exec/non-grouping-aggregator.h M be/src/exec/partitioned-hash-join-builder.cc M be/src/exec/partitioned-hash-join-builder.h M be/src/exec/partitioned-hash-join-node.cc M be/src/exprs/expr-test.cc M be/src/exprs/scalar-expr.cc M be/src/exprs/scalar-expr.h 15 files changed, 451 insertions(+), 213 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/53/15053/4 -- To view, visit http://gerrit.cloudera.org:8080/15053 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I58f52a262ac7d0af259d5bcda72ada93a851d3b2 Gerrit-Change-Number: 15053 Gerrit-PatchSet: 4 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Daniel Becker Gerrit-Reviewer: Tim Armstrong
