Steve Carlin has posted comments on this change. ( http://gerrit.cloudera.org:8080/24597 )
Change subject: IMPALA-7996: Optimize constant-false outer joins ...................................................................... Patch Set 3: (4 comments) http://gerrit.cloudera.org:8080/#/c/24597/3/fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java File fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java: http://gerrit.cloudera.org:8080/#/c/24597/3/fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java@459 PS3, Line 459: boolean useRandomPartitioning = node.isConstantFalseOuterJoin(); So it's good to prevent data skew and I wouldn't hold up the review for this since it makes the code better. I've gotten a little education very recently on broadcast versus partitioned join on the feature I'm working on. And what I understand is that the broadcast is generally preferred because it prevents an extra exchange that the partitioned join needs. So going back a level here: Would we ever even want to use a partitioned join in this case? I know your test case explicitly uses the join hint "shuffle", but maybe it would be better in this case to ignore the hint since there really is no benefit to using a partitioned join here. http://gerrit.cloudera.org:8080/#/c/24597/3/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java File fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java: http://gerrit.cloudera.org:8080/#/c/24597/3/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@2118 PS3, Line 2118: e -> Expr.IS_FALSE_LITERAL.apply(e) || Expr.IS_NULL_VALUE.apply(e)); I guess this is ok and safer to go through all the conjuncts , but I do wonder if it's necessary. You'd figure that somewhere in the analysis step, there would be a simplification of "a = b AND false" that would change the expression to "false". Thoughts? http://gerrit.cloudera.org:8080/#/c/24597/3/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@2121 PS3, Line 2121: eqJoinConjuncts.add(createNonMatchingEqJoinConjunct(analyzer)); So the join conjunct which prolly gets classified in "other" will contain the "false". I'm curious what the specific need is to move it into the "equiJoinConjuncts" http://gerrit.cloudera.org:8080/#/c/24597/3/testdata/workloads/functional-planner/queries/PlannerTest/empty.test File testdata/workloads/functional-planner/queries/PlannerTest/empty.test: http://gerrit.cloudera.org:8080/#/c/24597/3/testdata/workloads/functional-planner/queries/PlannerTest/empty.test@162 PS3, Line 162: | other join predicates: FALSE I referred to this in my other comment, but if we wouldn't need two predicates that are both false. -- To view, visit http://gerrit.cloudera.org:8080/24597 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6505825fc958720eec12e13d546661834f221b80 Gerrit-Change-Number: 24597 Gerrit-PatchSet: 3 Gerrit-Owner: Aleksandr Efimov <[email protected]> Gerrit-Reviewer: Aleksandr Efimov <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Paul Rogers <[email protected]> Gerrit-Reviewer: Steve Carlin <[email protected]> Gerrit-Reviewer: Yifan Zhang <[email protected]> Gerrit-Comment-Date: Wed, 29 Jul 2026 17:08:18 +0000 Gerrit-HasComments: Yes
