Steve Carlin has posted comments on this change. ( http://gerrit.cloudera.org:8080/24605 )
Change subject: IMPALA-15195: Calcite Planner performance improvements for tpcds ...................................................................... Patch Set 6: (6 comments) http://gerrit.cloudera.org:8080/#/c/24605/5/fe/src/main/java/org/apache/impala/planner/Planner.java File fe/src/main/java/org/apache/impala/planner/Planner.java: http://gerrit.cloudera.org:8080/#/c/24605/5/fe/src/main/java/org/apache/impala/planner/Planner.java@986 PS5, Line 986: lan when true. Th > nit: the naming of this parameter could be improved. From the related comm Done http://gerrit.cloudera.org:8080/#/c/24605/5/fe/src/main/java/org/apache/impala/planner/Planner.java@1007 PS5, Line 1007: root.computeTupleIds(); : return; > This comment says avoid invert joins for Calcite planner but the first 2 if Done http://gerrit.cloudera.org:8080/#/c/24605/5/fe/src/main/java/org/apache/impala/planner/SingleNodePlannerIntf.java File fe/src/main/java/org/apache/impala/planner/SingleNodePlannerIntf.java: http://gerrit.cloudera.org:8080/#/c/24605/5/fe/src/main/java/org/apache/impala/planner/SingleNodePlannerIntf.java@58 PS5, Line 58: /** > Add a comment. Done http://gerrit.cloudera.org:8080/#/c/24605/5/java/calcite-planner/src/main/java/org/apache/impala/calcite/schema/FilterSelectivityEstimator.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/schema/FilterSelectivityEstimator.java: http://gerrit.cloudera.org:8080/#/c/24605/5/java/calcite-planner/src/main/java/org/apache/impala/calcite/schema/FilterSelectivityEstimator.java@59 PS5, Line 59: private static final double DEFAULT_IS_NULL_PERCENTAGE = .02; > Is this default value based on certain benchmark characteristics ? Does Ca This is kind of an arbitrary value, but one that was discussed in a meeting before this commit. I couldn't find what Calcite does for this. Looking in their code, it seems that everything is user determined. the Filter getSelectivity() just essentially calls the child getSelectivity(), but allows application overrides, which is what we are doing. http://gerrit.cloudera.org:8080/#/c/24605/5/java/calcite-planner/src/main/java/org/apache/impala/calcite/schema/ImpalaCost.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/schema/ImpalaCost.java: http://gerrit.cloudera.org:8080/#/c/24605/5/java/calcite-planner/src/main/java/org/apache/impala/calcite/schema/ImpalaCost.java@129 PS5, Line 129: || ((Math.abs(this.cpu - that.cpu) < RelOptUtil.EPSILON) : && (Math.abs(this.io - that > The epsilon value is now pretty small. In the past, if this was motivated b q17 changed for a variety of other reasons, due to the changes in this patch. This comment is no longer relevant. http://gerrit.cloudera.org:8080/#/c/24605/5/java/calcite-planner/src/main/java/org/apache/impala/calcite/schema/ImpalaCost.java@144 PS5, Line 144: if (this == INFINITY) { > I understand this formula has some history and the perf experiments have be I changed this to use the same formula as the VolcanoPlanner, though I did remove the "rowCount" from the equation since we removed this in ImpalaCost. Only one plan changed, 14b. Given that so little was affected, I think it makes sense to make this change. I'll have to retest 14b, but even if this does have a regression, this commit still makes the plans better overall and the possible regression should be solvable in other ways. -- To view, visit http://gerrit.cloudera.org:8080/24605 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0ee6662dc6500ea6ebd9f859de4df1740c51a058 Gerrit-Change-Number: 24605 Gerrit-PatchSet: 6 Gerrit-Owner: Steve Carlin <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Steve Carlin <[email protected]> Gerrit-Comment-Date: Mon, 03 Aug 2026 22:43:00 +0000 Gerrit-HasComments: Yes
