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<ScalarExpr*>& 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 <[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: Thu, 30 Jan 2020 19:27:15 +0000 Gerrit-HasComments: Yes
