ankitsultana commented on code in PR #11976: URL: https://github.com/apache/pinot/pull/11976#discussion_r1388733745
########## pinot-query-planner/src/main/java/org/apache/calcite/rel/rules/PinotRelDistributionTraitRule.java: ########## @@ -0,0 +1,178 @@ +/** + * 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.calcite.rel.rules; + +import com.google.common.collect.ImmutableList; +import java.util.ArrayList; +import java.util.List; +import org.apache.calcite.plan.RelOptRule; +import org.apache.calcite.plan.RelOptRuleCall; +import org.apache.calcite.plan.RelTrait; +import org.apache.calcite.plan.RelTraitSet; +import org.apache.calcite.rel.RelDistribution; +import org.apache.calcite.rel.RelDistributions; +import org.apache.calcite.rel.RelNode; +import org.apache.calcite.rel.core.Exchange; +import org.apache.calcite.rel.hint.PinotHintOptions; +import org.apache.calcite.rel.hint.PinotHintStrategyTable; +import org.apache.calcite.rel.logical.LogicalAggregate; +import org.apache.calcite.rel.logical.LogicalFilter; +import org.apache.calcite.rel.logical.LogicalJoin; +import org.apache.calcite.rel.logical.LogicalProject; +import org.apache.calcite.rel.logical.LogicalTableScan; +import org.apache.calcite.rel.logical.PinotLogicalExchange; +import org.apache.calcite.tools.RelBuilderFactory; +import org.apache.calcite.util.mapping.Mappings; +import org.apache.pinot.query.planner.plannode.AggregateNode; +import org.checkerframework.checker.nullness.qual.Nullable; + + +/** + * Special rule for Pinot, this rule populates {@link RelDistribution} across the entire relational tree. + * + * we implement this rule as a workaround b/c {@link org.apache.calcite.plan.RelTraitPropagationVisitor}, which is + * deprecated. The idea is to associate every node with a RelDistribution derived from {@link RelNode#getInputs()} + * or from the node itself (via hints, or special handling of the type of node in question). + */ +public class PinotRelDistributionTraitRule extends RelOptRule { Review Comment: I had [worked on something similar](https://github.com/ankitsultana/pinot/pull/32/files#diff-46111cf490dd98bb30b046c546153f16c8e2cd569fd9c9be987a0665722c81f0) earlier this year, and had come to the conclusion that Calcite's RelDistribution and Exchange are not good enough for some of our use-cases because of the following reasons: - A given `RelNode` can practically have multiple `RelDistribution`. e.g. in a join node where both inputs are table-scan and partitioned by the join-key, the join node can be said to be distributed on both `LeftTable.key` and `RightTable.key`. But given how RelDistribution is, we can only keep information about one of the keys. This is in spite of the fact that RelDistribution is a RelMultipleTrait. For some reason the TraitSet only keeps one RelDistribution (I forgot the reasoning for this) - I had to also build my own Exchange nodes, because iirc I wanted to have "Identity" exchange support (i.e. the scenario where shuffle is not needed and partitions in input can be mapped 1:1 to partitions in output). In the linked PR above you can refer to the JSON Test File changes to see how the plan changes after my changes. I remember that I had gotten all the UTs working. I had abandoned this at the time because some of the other component design was not finalized so it was hard to get consensus on this big a change. We can discuss this in a call perhaps. -- 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]
