xiangfu0 commented on code in PR #18387: URL: https://github.com/apache/pinot/pull/18387#discussion_r3201306173
########## pinot-query-planner/src/main/java/org/apache/pinot/query/planner/rules/PinotRuleSet.java: ########## @@ -0,0 +1,100 @@ +/** + * 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.rules; + +import java.util.ArrayList; +import java.util.Collections; +import java.util.EnumMap; +import java.util.List; +import java.util.Map; +import java.util.ServiceLoader; +import org.apache.calcite.plan.RelOptRule; + + +/// Owns the multi-stage planner's per-phase Calcite rule lists. Constructed +/// once at broker startup; lists are immutable for the process lifetime. +/// +/// Construction sequence: +/// +/// 1. Allocate an empty mutable list per [Phase]. +/// 2. For every [RuleSetCustomizer] (in supplied order), call +/// `customize(phase, list)` once per phase. The OSS defaults are themselves +/// contributed by [DefaultRuleSetCustomizer] which is registered as a +/// `ServiceLoader` entry; plugin customizers run after and observe a list +/// pre-populated with the OSS defaults. +/// 3. Defensively copy each per-phase list to an immutable [List] and freeze +/// the map. +/// +/// `QueryEnvironment` reads `rulesFor(phase)` and applies per-query +/// `usePlannerRules` / `skipPlannerRules` filters on top. +public final class PinotRuleSet { + + private final Map<Phase, List<RelOptRule>> _rulesByPhase; + + /// Builds a rule set from the supplied customizers. Customizers run in the + /// order of the iterable; the framework freezes the per-phase lists after + /// every customizer has run. + public PinotRuleSet(Iterable<RuleSetCustomizer> customizers) { + EnumMap<Phase, List<RelOptRule>> mutable = new EnumMap<>(Phase.class); + for (Phase phase : Phase.values()) { + mutable.put(phase, new ArrayList<>()); + } + for (RuleSetCustomizer customizer : customizers) { + for (Phase phase : Phase.values()) { + customizer.customize(phase, mutable.get(phase)); + } + } + EnumMap<Phase, List<RelOptRule>> frozen = new EnumMap<>(Phase.class); + for (Map.Entry<Phase, List<RelOptRule>> entry : mutable.entrySet()) { + frozen.put(entry.getKey(), List.copyOf(entry.getValue())); + } + _rulesByPhase = Collections.unmodifiableMap(frozen); + } + + /// Discovers every [RuleSetCustomizer] via [ServiceLoader] and builds a + /// rule set from them. Used by [#defaultInstance()] and by callers that + /// don't have an externally-managed customizer list. + public static PinotRuleSet loadFromServiceLoader() { Review Comment: Even if a plugin adds the extra class-realm import, this discovery path only does `ServiceLoader.load(RuleSetCustomizer.class)`. Broker startup does not enumerate Pinot plugin classloaders here, and `PluginClassLoader` keeps plugin jars isolated, so third-party customizers will never be discovered and only `DefaultRuleSetCustomizer` will run. Please load providers from `PluginManager`/plugin realms explicitly instead of relying on the default `ServiceLoader` lookup. ########## pinot-query-planner/src/main/java/org/apache/pinot/query/planner/rules/RuleSetCustomizer.java: ########## @@ -0,0 +1,81 @@ +/** + * 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.rules; + +import java.util.List; +import org.apache.calcite.plan.RelOptRule; + + +/// Plugin SPI for customizing the multi-stage planner's Calcite rule sets. Review Comment: This is documented as a plugin SPI, but Pinot plugin realms only import `org.apache.pinot.spi` by default. A standard plugin packaged with `pinot-plugin.properties` cannot even link `org.apache.pinot.query.planner.rules.RuleSetCustomizer` without extra realm-import wiring, so the advertised extension point is broken for normal plugins. Please move this SPI into `pinot-spi` or import this package by default before landing. ########## pinot-query-planner/src/main/java/org/apache/pinot/query/planner/rules/DefaultRuleSetCustomizer.java: ########## @@ -43,26 +43,44 @@ import org.apache.calcite.rel.rules.SortJoinTransposeRule; import org.apache.calcite.rel.rules.SortRemoveRule; import org.apache.calcite.rel.rules.UnionToDistinctRule; +import org.apache.pinot.calcite.rel.rules.PinotAggregateExchangeNodeInsertRule; +import org.apache.pinot.calcite.rel.rules.PinotAggregateFunctionRewriteRule; +import org.apache.pinot.calcite.rel.rules.PinotAggregateReduceFunctionsRule; +import org.apache.pinot.calcite.rel.rules.PinotEnrichedJoinRule; +import org.apache.pinot.calcite.rel.rules.PinotEvaluateLiteralRule; +import org.apache.pinot.calcite.rel.rules.PinotExchangeEliminationRule; import org.apache.pinot.calcite.rel.rules.PinotFilterJoinRule.PinotFilterIntoJoinRule; import org.apache.pinot.calcite.rel.rules.PinotFilterJoinRule.PinotJoinConditionPushRule; +import org.apache.pinot.calcite.rel.rules.PinotJoinExchangeNodeInsertRule; +import org.apache.pinot.calcite.rel.rules.PinotJoinPushTransitivePredicatesRule; +import org.apache.pinot.calcite.rel.rules.PinotJoinToDynamicBroadcastRule; +import org.apache.pinot.calcite.rel.rules.PinotLogicalAggregateRule; +import org.apache.pinot.calcite.rel.rules.PinotProjectJoinTransposeRule; +import org.apache.pinot.calcite.rel.rules.PinotSemiJoinDistinctProjectRule; +import org.apache.pinot.calcite.rel.rules.PinotSetOpExchangeNodeInsertRule; +import org.apache.pinot.calcite.rel.rules.PinotSingleValueAggregateRemoveRule; +import org.apache.pinot.calcite.rel.rules.PinotSortExchangeCopyRule; +import org.apache.pinot.calcite.rel.rules.PinotSortExchangeNodeInsertRule; +import org.apache.pinot.calcite.rel.rules.PinotTableScanConverterRule; +import org.apache.pinot.calcite.rel.rules.PinotWindowExchangeNodeInsertRule; +import org.apache.pinot.calcite.rel.rules.PinotWindowSplitRule; import org.apache.pinot.spi.utils.CommonConstants.Broker.PlannerRuleNames; -/** - * Default rule sets for Pinot query - * Defaultly disabled rules are defined in - * {@link org.apache.pinot.spi.utils.CommonConstants.Broker#DEFAULT_DISABLED_RULES} - * - * TODO: This class started as a list of constant rule sets, but since then we have added dynamic rule generation - * to it as well. We should probably refactor the class to make it easier to understand, maintain and change the rules - * based on contextual information like query options. - */ -public class PinotQueryRuleSets { - private PinotQueryRuleSets() { - } +/// [RuleSetCustomizer] that seeds every [Phase] with the OSS default Calcite +/// rules for the multi-stage query planner. Registered as a [java.util.ServiceLoader] +/// service entry so it is picked up automatically by [PinotRuleSet]. +/// +/// `POST_LOGICAL` includes `PinotSortExchangeCopyRule.SORT_EXCHANGE_COPY` +/// configured with the rule's hard-coded default `fetchLimitThreshold`. +/// Per-query overrides (and broker-config overrides) of that limit are +/// applied later by `QueryEnvironment.getTraitProgram`, which swaps the +/// configured `PinotSortExchangeCopyRule` on a per-query copy of the +/// `POST_LOGICAL` list. +public final class DefaultRuleSetCustomizer implements RuleSetCustomizer { Review Comment: This rename removes the existing public `org.apache.pinot.calcite.rel.rules.PinotQueryRuleSets` type entirely. Any out-of-tree planner extension or test compiled against current releases will fail to load or compile after upgrade. Please keep a deprecated bridge in the old package instead of replacing the class outright. -- 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]
