Aman Sinha has posted comments on this change. ( http://gerrit.cloudera.org:8080/20104 )
Change subject: IMPALA-11842: Improve memory estimation for Aggregate ...................................................................... Patch Set 12: Code-Review+1 (4 comments) Just a few comments below. Overall, looks good ! http://gerrit.cloudera.org:8080/#/c/20104/12/common/thrift/ImpalaService.thrift File common/thrift/ImpalaService.thrift: http://gerrit.cloudera.org:8080/#/c/20104/12/common/thrift/ImpalaService.thrift@816 PS12, Line 816: node nit: to be precise can we say 'aggregation node's memory estimate' is deemed .. http://gerrit.cloudera.org:8080/#/c/20104/12/common/thrift/ImpalaService.thrift@817 PS12, Line 817: node nit: same as above http://gerrit.cloudera.org:8080/#/c/20104/12/common/thrift/ImpalaService.thrift@819 PS12, Line 819: lower shouldn't this be 'higher' ? i.e new memory estimate will not be higher than the LARGE_AGG_MEM_THRESHOLD .. http://gerrit.cloudera.org:8080/#/c/20104/12/fe/src/main/java/org/apache/impala/planner/AggregationNode.java File fe/src/main/java/org/apache/impala/planner/AggregationNode.java: http://gerrit.cloudera.org:8080/#/c/20104/12/fe/src/main/java/org/apache/impala/planner/AggregationNode.java@682 PS12, Line 682: aggInfo.getGroupingExprs().size() In some queries, we have seen several constant literals also included in the grouping columns. E.g GROUP BY col1, col2, 'abc', 'def' or for the ROLLUP case, there could be NULL: e.g GROUP BY col1, col2, NULL, NULL In such cases, the correlation adjustment should ideally only consider the actual columns, not the constants or NULLs. If you agree, it is ok to do this in a follow up patch since the tests are already passing and I don't think this should hold it up. -- To view, visit http://gerrit.cloudera.org:8080/20104 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia4b4b2e519ee89f0a13fdb62d0471ee4047f6421 Gerrit-Change-Number: 20104 Gerrit-PatchSet: 12 Gerrit-Owner: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Reviewer: Abhishek Rawat <ara...@cloudera.com> Gerrit-Reviewer: Aman Sinha <amsi...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Kurt Deschler <kdesc...@cloudera.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Reviewer: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Comment-Date: Wed, 12 Jul 2023 23:20:32 +0000 Gerrit-HasComments: Yes
