Abhishek Rawat has posted comments on this change. ( http://gerrit.cloudera.org:8080/21762 )
Change subject: IMPALA-13333: Limit memory estimation if PlanNode can spill ...................................................................... Patch Set 35: (4 comments) http://gerrit.cloudera.org:8080/#/c/21762/35//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21762/35//COMMIT_MSG@25 PS35, Line 25: and nit: or http://gerrit.cloudera.org:8080/#/c/21762/33/common/thrift/ImpalaService.thrift File common/thrift/ImpalaService.thrift: http://gerrit.cloudera.org:8080/#/c/21762/33/common/thrift/ImpalaService.thrift@1018 PS33, Line 1018: of them is set. : // Setting to 0.0 or unsetting scratch_dirs flag will re > I prefer to leave out 0.0 if it is OK. I think this is okay if explained clearly. http://gerrit.cloudera.org:8080/#/c/21762/33/fe/src/main/java/org/apache/impala/analysis/Analyzer.java File fe/src/main/java/org/apache/impala/analysis/Analyzer.java: http://gerrit.cloudera.org:8080/#/c/21762/33/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@765 PS33, Line 765: > Done. Although, I might need to wait until https://gerrit.cloudera.org/c/22 It can be handled as part of that patch. http://gerrit.cloudera.org:8080/#/c/21762/35/fe/src/main/java/org/apache/impala/analysis/Analyzer.java File fe/src/main/java/org/apache/impala/analysis/Analyzer.java: http://gerrit.cloudera.org:8080/#/c/21762/35/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@805 PS35, Line 805: RequestPoolService poolService = RequestPoolService.getInstance(); : if (poolService != null && groupSet.getMax_mem_limit() > 0) { : TPoolConfig poolConfig = : poolService.getPoolConfig(groupSet.getExec_group_name_prefix()); : if (poolConfig.isClamp_mem_limit_query_option()) { : globalState_.poolMemLimit_ = groupSet.getMax_mem_limit(); : } : } : } I think the logic needs to be updated because it's being used for both mem_limit is set and not set cases. When mem_limit query option is not set then max_query_mem_limit applies to query memory estimation. If mem_limit query option is set, then it's clamped by max_query_mem_limit only if clamp-mem-limit-query-option is set. I think removing the check for 'if (poolConfig.isClamp_mem_limit_query_option())' in setPoolMemLimit() and including that check in the 'mem_limit' case in getMaxMemLimitPerHost should address this: if (globalState_.poolMemLimit_ > 0 && poolConfig.isClamp_mem_limit_query_option()) { // clamp-mem-limit-query-option is True and max-query-mem-limit is set. // max-query-mem-limit take precedence over MEM_LIMIT if it is lower. mem_limit = Math.min(mem_limit, globalState_.poolMemLimit_); } -- To view, visit http://gerrit.cloudera.org:8080/21762 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I290c4e889d4ab9e921e356f0f55a9c8b11d0854e Gerrit-Change-Number: 21762 Gerrit-PatchSet: 35 Gerrit-Owner: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Reviewer: Abhishek Rawat <ara...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Daniel Becker <daniel.bec...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com> Gerrit-Reviewer: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com> Gerrit-Reviewer: Yida Wu <wydbaggio...@gmail.com> Gerrit-Comment-Date: Mon, 10 Mar 2025 01:33:40 +0000 Gerrit-HasComments: Yes
