Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/22897 )
Change subject: IMPALA-14071: Refactor MathUtil.saturatingMultiplyCardinalities() ...................................................................... Patch Set 8: (2 comments) Will address other commits in next patch set. http://gerrit.cloudera.org:8080/#/c/22897/8/fe/src/main/java/org/apache/impala/util/MathUtil.java File fe/src/main/java/org/apache/impala/util/MathUtil.java: http://gerrit.cloudera.org:8080/#/c/22897/8/fe/src/main/java/org/apache/impala/util/MathUtil.java@39 PS8, Line 39: } > Does it make sense to have saturatingMultiple and saturatingAdd anymore? It is not, but I rather clean this up in later patch to reduce change size and ensure that this change is correct. If we insist to remove this now, I want to run exhaustive test first for safety. http://gerrit.cloudera.org:8080/#/c/22897/8/testdata/workloads/functional-planner/queries/PlannerTest/resource-requirements.test File testdata/workloads/functional-planner/queries/PlannerTest/resource-requirements.test: http://gerrit.cloudera.org:8080/#/c/22897/8/testdata/workloads/functional-planner/queries/PlannerTest/resource-requirements.test@978 PS8, Line 978: Per-Host Resource Estimates: Memory=85MB > Are the new estimates more accurate? Impala already has a tendency to over I think it is better to be correct here. Existing calculation is already wrong because of negative values. The 10MB is coming from exchg_node_buffer_size_bytes flag. It is tunable to lower value, but currently default to 10MB. The original commit also mention that this might be too big, but I prefer we have proper benchmark first before changing it. https://gerrit.cloudera.org/c/11692/8/fe/src/main/java/org/apache/impala/planner/ExchangeNode.java#250 -- To view, visit http://gerrit.cloudera.org:8080/22897 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I505ab11cfa1024feb4ceac4cffe9c3283be228ce Gerrit-Change-Number: 22897 Gerrit-PatchSet: 8 Gerrit-Owner: Riza Suminto <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Reviewer: Wenzhe Zhou <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Wed, 14 May 2025 19:39:25 +0000 Gerrit-HasComments: Yes
