Jason Fehr has posted comments on this change. ( http://gerrit.cloudera.org:8080/22897 )
Change subject: IMPALA-14071: Refactor MathUtil.saturatingMultiplyCardinalities() ...................................................................... Patch Set 8: (18 comments) http://gerrit.cloudera.org:8080/#/c/22897/8//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/22897/8//COMMIT_MSG@38 PS8, Line 38: - Pass CardinalityTest and MathUtilTest. Need to mention the workload tests too. http://gerrit.cloudera.org:8080/#/c/22897/3/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/22897/3/fe/src/main/java/org/apache/impala/planner/AggregationNode.java@449 PS3, Line 449: MathUtil.multip > Done Done http://gerrit.cloudera.org:8080/#/c/22897/3/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java File fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java: http://gerrit.cloudera.org:8080/#/c/22897/3/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@2814 PS3, Line 2814: tatsConjunct in > Done Done http://gerrit.cloudera.org:8080/#/c/22897/8/fe/src/main/java/org/apache/impala/planner/PlanNode.java File fe/src/main/java/org/apache/impala/planner/PlanNode.java: http://gerrit.cloudera.org:8080/#/c/22897/8/fe/src/main/java/org/apache/impala/planner/PlanNode.java@87 PS8, Line 87: public final static Logger LOG = LoggerFactory.getLogger(PlanNode.class); I don't see where this is being used in this class. Thus, it should be removed instead of marking it public. 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? http://gerrit.cloudera.org:8080/#/c/22897/3/fe/src/test/java/org/apache/impala/util/MathUtilTest.java File fe/src/test/java/org/apache/impala/util/MathUtilTest.java: http://gerrit.cloudera.org:8080/#/c/22897/3/fe/src/test/java/org/apache/impala/util/MathUtilTest.java@49 PS3, Line 49: idCard = {0, 1, 2, Long.MAX_VALUE > Done Done http://gerrit.cloudera.org:8080/#/c/22897/8/fe/src/test/java/org/apache/impala/util/MathUtilTest.java File fe/src/test/java/org/apache/impala/util/MathUtilTest.java: http://gerrit.cloudera.org:8080/#/c/22897/8/fe/src/test/java/org/apache/impala/util/MathUtilTest.java@56 PS8, Line 56: MathUtil.saturatingMultiply(c1, c2) Nit: if possible it would be better to call LongMath.saturatedMultiply so that our code is completely eliminated from calculating the expected value. Another option would be to have an array with the expected values hardcoded in this test. http://gerrit.cloudera.org:8080/#/c/22897/8/fe/src/test/java/org/apache/impala/util/MathUtilTest.java@75 PS8, Line 75: // expected Need to assert e.getMessage() http://gerrit.cloudera.org:8080/#/c/22897/8/fe/src/test/java/org/apache/impala/util/MathUtilTest.java@79 PS8, Line 79: invalid This argument should be unknown instead of invalid. This case currently is retesting case 4. http://gerrit.cloudera.org:8080/#/c/22897/8/fe/src/test/java/org/apache/impala/util/MathUtilTest.java@82 PS8, Line 82: // expected Need to assert e.getMessage() http://gerrit.cloudera.org:8080/#/c/22897/8/fe/src/test/java/org/apache/impala/util/MathUtilTest.java@110 PS8, Line 110: MathUtil.saturatingAdd Nit: if possible it would be better to call LongMath.saturatedAdd so that our code is completely eliminated from calculating the expected value. Another option would be to have an array with the expected values hardcoded in this test. http://gerrit.cloudera.org:8080/#/c/22897/8/fe/src/test/java/org/apache/impala/util/MathUtilTest.java@127 PS8, Line 127: // expected Need to assert e.getMessage() http://gerrit.cloudera.org:8080/#/c/22897/8/fe/src/test/java/org/apache/impala/util/MathUtilTest.java@131 PS8, Line 131: invalid This argument should be unknown instead of invalid. This case currently is retesting case 4. http://gerrit.cloudera.org:8080/#/c/22897/8/fe/src/test/java/org/apache/impala/util/MathUtilTest.java@134 PS8, Line 134: // expected Need to assert e.getMessage() http://gerrit.cloudera.org:8080/#/c/22897/8/fe/src/test/java/org/apache/impala/util/MathUtilTest.java@166 PS8, Line 166: // expected Need to assert e.getMessage() http://gerrit.cloudera.org:8080/#/c/22897/8/fe/src/test/java/org/apache/impala/util/MathUtilTest.java@170 PS8, Line 170: MathUtil.smallestValidCardinality(invalid, invalid); This argument should be unknown instead of invalid. This case currently is retesting case 4. http://gerrit.cloudera.org:8080/#/c/22897/8/fe/src/test/java/org/apache/impala/util/MathUtilTest.java@173 PS8, Line 173: // expected Need to assert e.getMessage() 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 estimate memory usage, and these workload test changes all (except a couple) increased the estimated memory some by an order of magnitude. -- 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:19:19 +0000 Gerrit-HasComments: Yes
