Aman Sinha has posted comments on this change. ( http://gerrit.cloudera.org:8080/24605 )
Change subject: IMPALA-15195: Calcite Planner performance improvements for tpcds ...................................................................... Patch Set 5: (6 comments) I haven't looked at all the plan changes but wanted to send a few comments as one of them would impact plans if we decide to address it. 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: invertCheaperJoin nit: the naming of this parameter could be improved. From the related comment below, it looks like we want to do the inversion for couple of specific scenarios but not for the general case. How about shouldCheckForInvertJoinCost or something similar. True for Impala and False for Calcite. http://gerrit.cloudera.org:8080/#/c/24605/5/fe/src/main/java/org/apache/impala/planner/Planner.java@1007 PS5, Line 1007: // IMPALA-15196: Calcite plan should avoid all invert joins in its : // optimization phase. This comment says avoid invert joins for Calcite planner but the first 2 if/else-if conditions below are not checking for the invertCheaperJoin flag, so in those cases the inversion will happen. I suppose that is intentional .. in that case pls update the comment. 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: boolean allowPlannerToInvertCheaperJoin(); Add a comment. 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 Calcite have a default for this ? 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: // changed the epsilon value to be higher. Without this, q17 : // was producing a bad plan. The epsilon value is now pretty small. In the past, if this was motivated by q17 which benefited from the higher epsilon, does that regress q17 now with this change ? 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: Math.abs(1.0 - (this.cpu + this.io) / (other.getCpu() + other.getIo())) < EPSILON; I understand this formula has some history and the perf experiments have been done with it but since we are in the process of improving certain query plans, it should be re-visited since this formula has a significant impact overall. In particular, it adds cpu and io and takes ratios, then subtracts from 1.0 .. but we should just compare cpu with other cpu and io with other io and if they are within the epsilon then they are equal. Can we simplify this ? For reference, see VolcanoPlanner's method: https://github.com/apache/calcite/blob/c0295dc6b4380ecee0a3935a6f971ff4deab4c8d/core/src/main/java/org/apache/calcite/plan/volcano/VolcanoCost.java#L144 -- 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: 5 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-Comment-Date: Mon, 03 Aug 2026 03:45:59 +0000 Gerrit-HasComments: Yes
