jacques-n commented on a change in pull request #2378: URL: https://github.com/apache/calcite/pull/2378#discussion_r742354266
########## File path: core/src/main/java/org/apache/calcite/rel/metadata/MetadataHandlerProvider.java ########## @@ -0,0 +1,52 @@ +/* + * 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.metadata; + +import org.apache.calcite.rel.RelNode; +import org.apache.calcite.util.ControlFlowException; + +/** + * Provides {@link MetadataHandler} call sites and {@link MetadataCache} for + * {@link RelMetadataQuery}. The handlers provided are responsible for + * updating the cache stored in {@link RelMetadataQuery}. + */ +public interface MetadataHandlerProvider { + + <MH extends MetadataHandler<?>> MH initialHandler(Class<MH> handlerClass); + + /** Re-generates the handler for a given kind of metadata.. */ + <MH extends MetadataHandler<?>> MH revise(Class<MH> handlerClass); + + /** + * Creates a new cache. + * + * @return A new cache for {@link RelMetadataQuery} + */ + MetadataCache buildCache(); Review comment: I don't really understand the meaning of this method. Why does someone call this? What happens if I call it multiple times? What is this caching? Does it have to be called? Why is it on this interface? Also, if you wanted to customize the cache for things like a double property, wouldn't you potentially want to implement a cache that avoided the constant creation of objects and boxing (which the interface of MetadataCache forces)? ########## File path: core/src/main/java/org/apache/calcite/rel/metadata/RelMetadataQueryBase.java ########## @@ -62,49 +62,56 @@ * </ol> */ public class RelMetadataQueryBase { + //~ Instance fields -------------------------------------------------------- /** Set of active metadata queries, and cache of previous results. */ - public final Table<RelNode, Object, Object> map = HashBasedTable.create(); + @Deprecated // to be removed before 2.0 + public final Table<RelNode, Object, Object> map; - public final @Nullable JaninoRelMetadataProvider metadataProvider; + @API(status = API.Status.INTERNAL) + public final MetadataCache cache; + protected final MetadataHandlerProvider metadataHandlerProvider; + + @Deprecated // to be removed before 2.0 + public final @Nullable JaninoRelMetadataProvider metadataProvider = THREAD_PROVIDERS.get(); //~ Static fields/initializers --------------------------------------------- + @Deprecated // to be removed before 2.0 public static final ThreadLocal<@Nullable JaninoRelMetadataProvider> THREAD_PROVIDERS = new ThreadLocal<>(); - //~ Constructors ----------------------------------------------------------- - Review comment: Isn't removing this constructor a breaking change? ########## File path: core/src/main/java/org/apache/calcite/rel/metadata/RelMetadataQuery.java ########## @@ -107,47 +109,70 @@ private BuiltInMetadata.LowerBoundCost.Handler lowerBoundCostHandler; /** - * Creates the instance with {@link JaninoRelMetadataProvider} instance - * from {@link #THREAD_PROVIDERS} and {@link #EMPTY} as a prototype. + * Creates the instance using the Handler.classualt prototype. */ + @Deprecated protected RelMetadataQuery() { - this(castNonNull(THREAD_PROVIDERS.get()), EMPTY); + this(PROTOTYPE); } /** Creates and initializes the instance that will serve as a prototype for * all other instances. */ - private RelMetadataQuery(@SuppressWarnings("unused") boolean dummy) { - super(null); - this.collationHandler = initialHandler(BuiltInMetadata.Collation.Handler.class); - this.columnOriginHandler = initialHandler(BuiltInMetadata.ColumnOrigin.Handler.class); - this.expressionLineageHandler = initialHandler(BuiltInMetadata.ExpressionLineage.Handler.class); - this.tableReferencesHandler = initialHandler(BuiltInMetadata.TableReferences.Handler.class); - this.columnUniquenessHandler = initialHandler(BuiltInMetadata.ColumnUniqueness.Handler.class); - this.cumulativeCostHandler = initialHandler(BuiltInMetadata.CumulativeCost.Handler.class); - this.distinctRowCountHandler = initialHandler(BuiltInMetadata.DistinctRowCount.Handler.class); - this.distributionHandler = initialHandler(BuiltInMetadata.Distribution.Handler.class); - this.explainVisibilityHandler = initialHandler(BuiltInMetadata.ExplainVisibility.Handler.class); - this.maxRowCountHandler = initialHandler(BuiltInMetadata.MaxRowCount.Handler.class); - this.minRowCountHandler = initialHandler(BuiltInMetadata.MinRowCount.Handler.class); - this.memoryHandler = initialHandler(BuiltInMetadata.Memory.Handler.class); - this.nonCumulativeCostHandler = initialHandler(BuiltInMetadata.NonCumulativeCost.Handler.class); - this.parallelismHandler = initialHandler(BuiltInMetadata.Parallelism.Handler.class); + protected RelMetadataQuery(MetadataHandlerProvider metadataHandlerProvider) { + super(metadataHandlerProvider); + this.collationHandler = + metadataHandlerProvider.initialHandler(BuiltInMetadata.Collation.Handler.class); + this.columnOriginHandler = + metadataHandlerProvider.initialHandler(BuiltInMetadata.ColumnOrigin.Handler.class); + this.expressionLineageHandler = + metadataHandlerProvider.initialHandler(BuiltInMetadata.ExpressionLineage.Handler.class); + this.tableReferencesHandler = + metadataHandlerProvider.initialHandler(BuiltInMetadata.TableReferences.Handler.class); + this.columnUniquenessHandler = + metadataHandlerProvider.initialHandler(BuiltInMetadata.ColumnUniqueness.Handler.class); + this.cumulativeCostHandler = + metadataHandlerProvider.initialHandler(BuiltInMetadata.CumulativeCost.Handler.class); + this.distinctRowCountHandler = + metadataHandlerProvider.initialHandler(BuiltInMetadata.DistinctRowCount.Handler.class); + this.distributionHandler = + metadataHandlerProvider.initialHandler(BuiltInMetadata.Distribution.Handler.class); + this.explainVisibilityHandler = + metadataHandlerProvider.initialHandler(BuiltInMetadata.ExplainVisibility.Handler.class); + this.maxRowCountHandler = + metadataHandlerProvider.initialHandler(BuiltInMetadata.MaxRowCount.Handler.class); + this.minRowCountHandler = + metadataHandlerProvider.initialHandler(BuiltInMetadata.MinRowCount.Handler.class); + this.memoryHandler = + metadataHandlerProvider.initialHandler(BuiltInMetadata.Memory.Handler.class); + this.nonCumulativeCostHandler = + metadataHandlerProvider.initialHandler(BuiltInMetadata.NonCumulativeCost.Handler.class); + this.parallelismHandler = + metadataHandlerProvider.initialHandler(BuiltInMetadata.Parallelism.Handler.class); this.percentageOriginalRowsHandler = - initialHandler(BuiltInMetadata.PercentageOriginalRows.Handler.class); - this.populationSizeHandler = initialHandler(BuiltInMetadata.PopulationSize.Handler.class); - this.predicatesHandler = initialHandler(BuiltInMetadata.Predicates.Handler.class); - this.allPredicatesHandler = initialHandler(BuiltInMetadata.AllPredicates.Handler.class); - this.nodeTypesHandler = initialHandler(BuiltInMetadata.NodeTypes.Handler.class); - this.rowCountHandler = initialHandler(BuiltInMetadata.RowCount.Handler.class); - this.selectivityHandler = initialHandler(BuiltInMetadata.Selectivity.Handler.class); - this.sizeHandler = initialHandler(BuiltInMetadata.Size.Handler.class); - this.uniqueKeysHandler = initialHandler(BuiltInMetadata.UniqueKeys.Handler.class); - this.lowerBoundCostHandler = initialHandler(BuiltInMetadata.LowerBoundCost.Handler.class); - } - - private RelMetadataQuery(JaninoRelMetadataProvider metadataProvider, - RelMetadataQuery prototype) { - super(requireNonNull(metadataProvider, "metadataProvider")); + metadataHandlerProvider.initialHandler( + BuiltInMetadata.PercentageOriginalRows.Handler.class); + this.populationSizeHandler = + metadataHandlerProvider.initialHandler(BuiltInMetadata.PopulationSize.Handler.class); + this.predicatesHandler = + metadataHandlerProvider.initialHandler(BuiltInMetadata.Predicates.Handler.class); + this.allPredicatesHandler = + metadataHandlerProvider.initialHandler(BuiltInMetadata.AllPredicates.Handler.class); + this.nodeTypesHandler = + metadataHandlerProvider.initialHandler(BuiltInMetadata.NodeTypes.Handler.class); + this.rowCountHandler = + metadataHandlerProvider.initialHandler(BuiltInMetadata.RowCount.Handler.class); + this.selectivityHandler = + metadataHandlerProvider.initialHandler(BuiltInMetadata.Selectivity.Handler.class); + this.sizeHandler = + metadataHandlerProvider.initialHandler(BuiltInMetadata.Size.Handler.class); + this.uniqueKeysHandler = + metadataHandlerProvider.initialHandler(BuiltInMetadata.UniqueKeys.Handler.class); + this.lowerBoundCostHandler = + metadataHandlerProvider.initialHandler(BuiltInMetadata.LowerBoundCost.Handler.class); + } + + protected RelMetadataQuery(RelMetadataQuery prototype) { Review comment: please make this private. ########## File path: core/src/main/java/org/apache/calcite/rel/metadata/MetadataHandlerProvider.java ########## @@ -0,0 +1,52 @@ +/* + * 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.metadata; + +import org.apache.calcite.rel.RelNode; +import org.apache.calcite.util.ControlFlowException; + +/** + * Provides {@link MetadataHandler} call sites and {@link MetadataCache} for + * {@link RelMetadataQuery}. The handlers provided are responsible for + * updating the cache stored in {@link RelMetadataQuery}. + */ +public interface MetadataHandlerProvider { Review comment: Config feels like it would be weird here. As I have thought more about the name I'm pretty much okay with it as is. ########## File path: core/src/main/java/org/apache/calcite/rel/metadata/RelMetadataQuery.java ########## @@ -107,47 +106,69 @@ private BuiltInMetadata.LowerBoundCost.Handler lowerBoundCostHandler; /** - * Creates the instance with {@link JaninoRelMetadataProvider} instance - * from {@link #THREAD_PROVIDERS} and {@link #EMPTY} as a prototype. + * Creates the instance using the Handler.classualt prototype. */ protected RelMetadataQuery() { - this(castNonNull(THREAD_PROVIDERS.get()), EMPTY); + this(PROTOTYPE); } /** Creates and initializes the instance that will serve as a prototype for * all other instances. */ - private RelMetadataQuery(@SuppressWarnings("unused") boolean dummy) { - super(null); - this.collationHandler = initialHandler(BuiltInMetadata.Collation.Handler.class); - this.columnOriginHandler = initialHandler(BuiltInMetadata.ColumnOrigin.Handler.class); - this.expressionLineageHandler = initialHandler(BuiltInMetadata.ExpressionLineage.Handler.class); - this.tableReferencesHandler = initialHandler(BuiltInMetadata.TableReferences.Handler.class); - this.columnUniquenessHandler = initialHandler(BuiltInMetadata.ColumnUniqueness.Handler.class); - this.cumulativeCostHandler = initialHandler(BuiltInMetadata.CumulativeCost.Handler.class); - this.distinctRowCountHandler = initialHandler(BuiltInMetadata.DistinctRowCount.Handler.class); - this.distributionHandler = initialHandler(BuiltInMetadata.Distribution.Handler.class); - this.explainVisibilityHandler = initialHandler(BuiltInMetadata.ExplainVisibility.Handler.class); - this.maxRowCountHandler = initialHandler(BuiltInMetadata.MaxRowCount.Handler.class); - this.minRowCountHandler = initialHandler(BuiltInMetadata.MinRowCount.Handler.class); - this.memoryHandler = initialHandler(BuiltInMetadata.Memory.Handler.class); - this.nonCumulativeCostHandler = initialHandler(BuiltInMetadata.NonCumulativeCost.Handler.class); - this.parallelismHandler = initialHandler(BuiltInMetadata.Parallelism.Handler.class); + protected RelMetadataQuery(MetadataHandlerProvider metadataHandlerProvider) { Review comment: I think I have a simple solution to avoiding the introduction of a new constructor on this class: - Make this constructor private - Add a new inner static subclass that uses that constructor and exposes a protected class. Then we tell people that want to use the handler behavior to extend that subclass. That way, we have deprecated subclassing RelMetadataQuery directly while moving towards this new pattern with minimal code changes. This avoids the introduction of a new coupling api between RelMetadataQuery the handler concept, is minimal changes in your patch and sets us on a path to moving RelMetadataQuery to an interface or dumb abstract class. ########## File path: core/src/main/java/org/apache/calcite/plan/RelOptCluster.java ########## @@ -136,7 +134,7 @@ public RexBuilder getRexBuilder() { return rexBuilder; } - public @Nullable RelMetadataProvider getMetadataProvider() { + public RelMetadataProvider getMetadataProvider() { Review comment: I see this marked resolved but don't see it being deprecated. ########## File path: core/src/main/java/org/apache/calcite/rel/metadata/RelMetadataQueryBase.java ########## @@ -63,49 +61,56 @@ * </ol> */ public class RelMetadataQueryBase { + //~ Instance fields -------------------------------------------------------- /** Set of active metadata queries, and cache of previous results. */ - public final Table<RelNode, List, Object> map = HashBasedTable.create(); - public final @Nullable JaninoRelMetadataProvider metadataProvider; + @Deprecated // to be removed before 2.0 + public final Table<RelNode, Object, Object> map; + + public final MetadataCache cache; + protected final MetadataHandlerProvider metadataHandlerProvider; Review comment: Please make metadataHandlerProvider private. I think this means that for now you need to move it to relmetadataquery and pass a cache supplier into the RelMetadataQueryBase Please make the cache private. I think using the Api pattern to "hide" things should be used as a last resort. This may mean it should also move to RelMetadataQuery (for now). -- 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]
