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<ScalarExpr*>& 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 <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Tue, 28 Jan 2020 14:23:07 +0000 Gerrit-HasComments: Yes
