gortiz commented on code in PR #14523: URL: https://github.com/apache/pinot/pull/14523#discussion_r1856010755
########## pinot-query-planner/src/main/java/org/apache/pinot/calcite/rel/rules/PinotFilterJoinRule.java: ########## @@ -0,0 +1,262 @@ +/** + * 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.pinot.calcite.rel.rules; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.Sets; +import java.util.ArrayList; +import java.util.List; +import org.apache.calcite.plan.RelOptRuleCall; +import org.apache.calcite.plan.RelOptUtil; +import org.apache.calcite.rel.RelNode; +import org.apache.calcite.rel.core.Filter; +import org.apache.calcite.rel.core.Join; +import org.apache.calcite.rel.core.JoinRelType; +import org.apache.calcite.rel.rules.FilterJoinRule; +import org.apache.calcite.rel.rules.FilterJoinRule.FilterIntoJoinRule.FilterIntoJoinRuleConfig; +import org.apache.calcite.rel.rules.FilterJoinRule.JoinConditionPushRule.JoinConditionPushRuleConfig; +import org.apache.calcite.rel.type.RelDataType; +import org.apache.calcite.rex.RexBuilder; +import org.apache.calcite.rex.RexCall; +import org.apache.calcite.rex.RexNode; +import org.apache.calcite.rex.RexUtil; +import org.apache.calcite.tools.RelBuilder; +import org.apache.pinot.calcite.rel.hint.PinotHintOptions; +import org.checkerframework.checker.nullness.qual.Nullable; + + +/** + * Similar to {@link FilterJoinRule} but do not push down filter into right side of lookup join. + */ +public abstract class PinotFilterJoinRule<C extends FilterJoinRule.Config> extends FilterJoinRule<C> { + + private PinotFilterJoinRule(C config) { + super(config); + } + + // Following code are copy-pasted from Calcite, and modified to not push down filter into right side of lookup join. + //@formatter:off + @Override + protected void perform(RelOptRuleCall call, @Nullable Filter filter, Join join) { + List<RexNode> joinFilters = + RelOptUtil.conjunctions(join.getCondition()); + final List<RexNode> origJoinFilters = ImmutableList.copyOf(joinFilters); + + // If there is only the joinRel, + // make sure it does not match a cartesian product joinRel + // (with "true" condition), otherwise this rule will be applied + // again on the new cartesian product joinRel. + if (filter == null && joinFilters.isEmpty()) { + return; + } + + final List<RexNode> aboveFilters = + filter != null + ? getConjunctions(filter) + : new ArrayList<>(); + final ImmutableList<RexNode> origAboveFilters = + ImmutableList.copyOf(aboveFilters); + + // Simplify Outer Joins + JoinRelType joinType = join.getJoinType(); + if (config.isSmart() + && !origAboveFilters.isEmpty() + && join.getJoinType() != JoinRelType.INNER) { + joinType = RelOptUtil.simplifyJoin(join, origAboveFilters, joinType); + } + + final List<RexNode> leftFilters = new ArrayList<>(); + final List<RexNode> rightFilters = new ArrayList<>(); + + // TODO - add logic to derive additional filters. E.g., from + // (t1.a = 1 AND t2.a = 2) OR (t1.b = 3 AND t2.b = 4), you can + // derive table filters: + // (t1.a = 1 OR t1.b = 3) + // (t2.a = 2 OR t2.b = 4) + + // PINOT MODIFICATION to not push down filter into right side of lookup join. + boolean canPushRight = !PinotHintOptions.JoinHintOptions.useLookupJoinStrategy(join); + + // Try to push down above filters. These are typically where clause + // filters. They can be pushed down if they are not on the NULL + // generating side. + boolean filterPushed = + RelOptUtil.classifyFilters(join, + aboveFilters, + joinType.canPushIntoFromAbove(), + joinType.canPushLeftFromAbove(), + canPushRight && joinType.canPushRightFromAbove(), + joinFilters, + leftFilters, + rightFilters); Review Comment: Probably we can contribute a patch in Calcite where `FilterJoinRule` calls an instance method to do this so we could just extend it. Something like: ```java public class FilterJoinRule extends ... { boolean classifyFilters( RelNode joinRel, List<RexNode> filters, JoinRelType relType, List<RexNode> joinFilters, List<RexNode> leftFilters, List<RexNode> rightFilters) { RelOptUtil.classifyFilters(join, aboveFilters, joinType.canPushIntoFromAbove(), joinType.canPushLeftFromAbove(), joinType.canPushRightFromAbove(), joinFilters, leftFilters, rightFilters); } } ``` So we could simplify this class by overriding that method and adding there our check. ########## pinot-query-planner/src/main/java/org/apache/pinot/calcite/rel/rules/PinotFilterJoinRule.java: ########## @@ -0,0 +1,262 @@ +/** + * 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.pinot.calcite.rel.rules; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.Sets; +import java.util.ArrayList; +import java.util.List; +import org.apache.calcite.plan.RelOptRuleCall; +import org.apache.calcite.plan.RelOptUtil; +import org.apache.calcite.rel.RelNode; +import org.apache.calcite.rel.core.Filter; +import org.apache.calcite.rel.core.Join; +import org.apache.calcite.rel.core.JoinRelType; +import org.apache.calcite.rel.rules.FilterJoinRule; +import org.apache.calcite.rel.rules.FilterJoinRule.FilterIntoJoinRule.FilterIntoJoinRuleConfig; +import org.apache.calcite.rel.rules.FilterJoinRule.JoinConditionPushRule.JoinConditionPushRuleConfig; +import org.apache.calcite.rel.type.RelDataType; +import org.apache.calcite.rex.RexBuilder; +import org.apache.calcite.rex.RexCall; +import org.apache.calcite.rex.RexNode; +import org.apache.calcite.rex.RexUtil; +import org.apache.calcite.tools.RelBuilder; +import org.apache.pinot.calcite.rel.hint.PinotHintOptions; +import org.checkerframework.checker.nullness.qual.Nullable; + + +/** + * Similar to {@link FilterJoinRule} but do not push down filter into right side of lookup join. + */ +public abstract class PinotFilterJoinRule<C extends FilterJoinRule.Config> extends FilterJoinRule<C> { + + private PinotFilterJoinRule(C config) { + super(config); + } + + // Following code are copy-pasted from Calcite, and modified to not push down filter into right side of lookup join. + //@formatter:off + @Override + protected void perform(RelOptRuleCall call, @Nullable Filter filter, Join join) { + List<RexNode> joinFilters = + RelOptUtil.conjunctions(join.getCondition()); + final List<RexNode> origJoinFilters = ImmutableList.copyOf(joinFilters); + + // If there is only the joinRel, + // make sure it does not match a cartesian product joinRel + // (with "true" condition), otherwise this rule will be applied + // again on the new cartesian product joinRel. + if (filter == null && joinFilters.isEmpty()) { + return; + } + + final List<RexNode> aboveFilters = + filter != null + ? getConjunctions(filter) + : new ArrayList<>(); + final ImmutableList<RexNode> origAboveFilters = + ImmutableList.copyOf(aboveFilters); + + // Simplify Outer Joins + JoinRelType joinType = join.getJoinType(); + if (config.isSmart() + && !origAboveFilters.isEmpty() + && join.getJoinType() != JoinRelType.INNER) { + joinType = RelOptUtil.simplifyJoin(join, origAboveFilters, joinType); + } + + final List<RexNode> leftFilters = new ArrayList<>(); + final List<RexNode> rightFilters = new ArrayList<>(); + + // TODO - add logic to derive additional filters. E.g., from + // (t1.a = 1 AND t2.a = 2) OR (t1.b = 3 AND t2.b = 4), you can + // derive table filters: + // (t1.a = 1 OR t1.b = 3) + // (t2.a = 2 OR t2.b = 4) + + // PINOT MODIFICATION to not push down filter into right side of lookup join. + boolean canPushRight = !PinotHintOptions.JoinHintOptions.useLookupJoinStrategy(join); + + // Try to push down above filters. These are typically where clause + // filters. They can be pushed down if they are not on the NULL + // generating side. + boolean filterPushed = + RelOptUtil.classifyFilters(join, + aboveFilters, + joinType.canPushIntoFromAbove(), + joinType.canPushLeftFromAbove(), + canPushRight && joinType.canPushRightFromAbove(), + joinFilters, + leftFilters, + rightFilters); Review Comment: Probably we can contribute a patch in Calcite where `FilterJoinRule` calls an instance method to do this so we could just extend it. Something like: ```java public class FilterJoinRule extends ... { boolean classifyFilters( RelNode joinRel, List<RexNode> filters, JoinRelType relType, List<RexNode> joinFilters, List<RexNode> leftFilters, List<RexNode> rightFilters) { RelOptUtil.classifyFilters(join, aboveFilters, joinType.canPushIntoFromAbove(), joinType.canPushLeftFromAbove(), joinType.canPushRightFromAbove(), joinFilters, leftFilters, rightFilters); } } ``` So we could simplify this class by overriding that method and adding there our check. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
