This is an automated email from the ASF dual-hosted git repository. michaelsmith pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/impala.git
commit 32cec3fd80b129e4d84a403f1ee0c4144101f80c Author: Steve Carlin <[email protected]> AuthorDate: Tue Oct 8 13:59:41 2024 -0700 IMPALA-13429: Calcite planner crashing with outer join The Calcite planner was crashing when there was an outer join and there was a conjunct that compared two columns within the same table. This conjunct needs to be place in "other" conjuncts rather than "equi" conjuncts. Change-Id: I4ae2d257fa58f3a58079b6aa551c32ffda7d28cf Reviewed-on: http://gerrit.cloudera.org:8080/21908 Reviewed-by: Michael Smith <[email protected]> Tested-by: Michael Smith <[email protected]> --- .../impala/calcite/rel/node/ImpalaJoinRel.java | 59 +++++++++++++++++++--- .../calcite/rel/util/RexInputRefCollector.java | 51 +++++++++++++++++++ 2 files changed, 104 insertions(+), 6 deletions(-) diff --git a/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaJoinRel.java b/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaJoinRel.java index bcaf7241b..4903c64a0 100644 --- a/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaJoinRel.java +++ b/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/node/ImpalaJoinRel.java @@ -46,6 +46,7 @@ import org.apache.impala.calcite.rel.phys.ImpalaHashJoinNode; import org.apache.impala.calcite.rel.phys.ImpalaNestedLoopJoinNode; import org.apache.impala.calcite.rel.util.CreateExprVisitor; import org.apache.impala.calcite.rel.util.ExprConjunctsConverter; +import org.apache.impala.calcite.rel.util.RexInputRefCollector; import org.apache.impala.calcite.type.ImpalaTypeConverter; import org.apache.impala.catalog.Function; import org.apache.impala.catalog.Type; @@ -57,6 +58,7 @@ import org.apache.impala.planner.PlanNode; import java.util.ArrayList; import java.util.HashSet; import java.util.List; +import java.util.Set; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -98,7 +100,7 @@ public class ImpalaJoinRel extends Join JoinOperator joinOp = getImpalaJoinOp(); List<BinaryPredicate> equiJoinConjuncts = new ArrayList<>(); - List<Expr> nonEquiJoinConjuncts = new ArrayList<>(); + List<Expr> otherJoinConjuncts = new ArrayList<>(); // IMPALA-13176: TODO: Impala allows forcing hints for the distribution mode // - e.g force broadcast or hash partition join. However, we are not @@ -119,7 +121,7 @@ public class ImpalaJoinRel extends Join if (conjunctInfo.isEquiJoin_) { equiJoinConjuncts.add((BinaryPredicate) conjunctInfo.conjunct_); } else { - nonEquiJoinConjuncts.add(conjunctInfo.conjunct_); + otherJoinConjuncts.add(conjunctInfo.conjunct_); } } } @@ -136,10 +138,10 @@ public class ImpalaJoinRel extends Join PlanNode joinNode = equiJoinConjuncts.size() == 0 ? new ImpalaNestedLoopJoinNode(context.ctx_.getNextNodeId(), leftInput.planNode_, rightInput.planNode_, false /* not a straight join */, distMode, joinOp, - nonEquiJoinConjuncts, filterConjuncts, analyzer) + otherJoinConjuncts, filterConjuncts, analyzer) : new ImpalaHashJoinNode(context.ctx_.getNextNodeId(), leftInput.planNode_, rightInput.planNode_, false /* not a straight join */, distMode, joinOp, - equiJoinConjuncts, nonEquiJoinConjuncts, filterConjuncts, analyzer); + equiJoinConjuncts, otherJoinConjuncts, filterConjuncts, analyzer); if (equiJoinConjuncts.size() > 0) { // register the equi and non-equi join conjuncts with the analyzer such that @@ -147,7 +149,7 @@ public class ImpalaJoinRel extends Join List<Expr> equiJoinExprs = new ArrayList<Expr>(equiJoinConjuncts); analyzer.registerConjuncts(getJoinConjunctListToRegister(equiJoinExprs)); } - analyzer.registerConjuncts(getJoinConjunctListToRegister(nonEquiJoinConjuncts)); + analyzer.registerConjuncts(getJoinConjunctListToRegister(otherJoinConjuncts)); return new NodeWithExprs(joinNode, outputExprs); } @@ -425,11 +427,56 @@ public class ImpalaJoinRel extends Join // get a canonicalized representation conj = getCanonical(conj, leftInput.outputExprs_.size()); Expr impalaConjunct = conj.accept(visitor); - conjunctInfos.add(new ConjunctInfo(impalaConjunct, conj.isA(SqlKind.EQUALS))); + conjunctInfos.add( + new ConjunctInfo(impalaConjunct, isEquijoinConjunct(conj, leftInput))); } return conjunctInfos; } + // Checks to see if this conjunct can be considered an "equijoin" conjunct. + // An equijoin conjunct must have the following traits: + // - should be an "EQUALS" RexCall at the top level. + // - be in canonical form, that is, the input refs on the left side of the + // equals RexNode should point to the left input and the right side of the + // equals RexNode should point to the right input. + // - should be at least one input ref on the left side and one input ref on + // the right side. + private boolean isEquijoinConjunct(RexNode conjunct, NodeWithExprs leftInput) { + if (!conjunct.isA(SqlKind.EQUALS)) { + return false; + } + + Preconditions.checkState(conjunct instanceof RexCall); + RexCall call = (RexCall) conjunct; + + // left side expr may only contain inputrefs from left side. + // right side expr may only contain inputrefs from right side. + RexNode left = call.getOperands().get(0); + Set<Integer> inputRefs = RexInputRefCollector.getInputRefs(left); + if (inputRefs.size() == 0) { + return false; + } + + for (Integer inputRef : inputRefs) { + if (inputRef >= leftInput.outputExprs_.size()) { + return false; + } + } + + RexNode right = call.getOperands().get(1); + inputRefs = RexInputRefCollector.getInputRefs(right); + if (inputRefs.size() == 0) { + return false; + } + for (Integer inputRef : inputRefs) { + if (inputRef < leftInput.outputExprs_.size()) { + return false; + } + } + return true; + + } + private static class ConjunctInfo { public final Expr conjunct_; public final boolean isEquiJoin_; diff --git a/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/util/RexInputRefCollector.java b/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/util/RexInputRefCollector.java new file mode 100644 index 000000000..f930cb435 --- /dev/null +++ b/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/util/RexInputRefCollector.java @@ -0,0 +1,51 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package org.apache.impala.calcite.rel.util; + +import java.util.HashSet; +import java.util.Set; + +import org.apache.calcite.rex.RexInputRef; +import org.apache.calcite.rex.RexNode; +import org.apache.calcite.rex.RexVisitorImpl; + +/** + * Collects all inputrefs for a given RexNodeExpr through the only + * public method, RexInputRefCollector.getInputRefs(RexNode). + */ +public class RexInputRefCollector extends RexVisitorImpl<Void> { + + private final Set<Integer> inputRefSet = new HashSet<Integer>(); + + private RexInputRefCollector(boolean deep) { + super(deep); + } + + @Override + public Void visitInputRef(RexInputRef inputRef) { + inputRefSet.add(inputRef.getIndex()); + return null; + } + + public static Set<Integer> getInputRefs(RexNode expr) { + RexInputRefCollector collector = new RexInputRefCollector(true); + expr.accept(collector); + return collector.inputRefSet; + } + +}
