gortiz commented on code in PR #14296: URL: https://github.com/apache/pinot/pull/14296#discussion_r1818733174
########## pinot-query-planner/src/main/java/org/apache/pinot/query/planner/logical/GroupedStages.java: ########## @@ -0,0 +1,177 @@ +/** + * 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.query.planner.logical; + +import com.google.common.base.Preconditions; +import java.util.Comparator; +import java.util.IdentityHashMap; +import java.util.NoSuchElementException; +import java.util.SortedSet; +import java.util.TreeSet; +import java.util.stream.Collectors; +import org.apache.pinot.query.planner.plannode.BasePlanNode; +import org.apache.pinot.query.planner.plannode.MailboxSendNode; + + +/** + * This represents a mathematical partition of the stages in a query plan, grouping the stages in sets of disjoint + * stages. + * + * It is important to understand that this class assumes all stages that are stored belong to the same query plan + * and therefore their stage ids are unique. It also assumes that the same stage instances are being used when + * methods like {@link #containsStage(MailboxSendNode)} are called. + * + * The original reason to have this class was to group equivalent stages together, although it can be used for other + * purposes. + * + * Although the only implementation provided so far ({@link Mutable}) is mutable, the class is designed + * to be immutable from the outside. This is because it is difficult to manipulate grouped stages directly without + * breaking the invariants of the class, so it is better to be sure it is not modified after it is calculated. + */ +public abstract class GroupedStages { Review Comment: I tried to explain that in lines 43 and 44. While the class is mutable while it is being build, it is immutable from the consumer perspective. The only way to do so is to have a not modifiable interface and a mutable class that implements it. This is why EquivalentStagesFinder.findEquivalentStages returns GroupStages (which is immutable) while internally uses the mutable class. The reason to do that is because keeping invariants is not trivial and we want to consume these objects as they were immutable. We already had problems in the past when complex objects were mutable from different parts of the code (see https://github.com/apache/pinot/issues/14106). Anyway, this is something we can change if needed. It is just subjective codestyle. In future PRs, once we start using this, it can be changed. For example, we are going to need to prune these GroupStages, removing stages that are already contained in a equivalent stage. But I didn't want to make this PR too complex -- 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: commits-unsubscr...@pinot.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
