jacques-n commented on a change in pull request #2378: URL: https://github.com/apache/calcite/pull/2378#discussion_r738683302
########## File path: core/src/main/java/org/apache/calcite/rel/metadata/janino/CacheGenerator.java ########## @@ -0,0 +1,423 @@ +/* + * 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.janino; + +import org.apache.calcite.linq4j.Ord; +import org.apache.calcite.linq4j.tree.Primitive; +import org.apache.calcite.rel.RelNode; +import org.apache.calcite.rel.metadata.CyclicMetadataException; +import org.apache.calcite.rel.metadata.DelegatingMetadataRel; +import org.apache.calcite.rel.metadata.NullSentinel; +import org.apache.calcite.rel.metadata.RelMetadataQuery; +import org.apache.calcite.rex.RexNode; +import org.apache.calcite.runtime.FlatLists; + +import com.google.common.collect.ImmutableList; + +import java.lang.reflect.Method; + +import static org.apache.calcite.rel.metadata.janino.CodeGeneratorUtil.argList; +import static org.apache.calcite.rel.metadata.janino.CodeGeneratorUtil.paramList; + +/** + * Generates caching code for janino backed metadata. + */ +class CacheGenerator { Review comment: Based on my initial analysis, it appears that all of this code can be removed from runtime generation. Is there a specific reason you don't think it can be? If you use objects throughout, I don't think it changes anything other than simplify things (and make them more debuggable). That being said, let's avoid coupling the generification of metadatquery versus the generification of a the janino impl (separate prs). (Part of the argument against a pre-compiled generic approach would be to avoid boxing. In this case however, I think that point is moot is the the column key will always be boxed. ########## File path: core/src/main/java/org/apache/calcite/plan/volcano/TopDownRuleDriver.java ########## @@ -440,7 +440,7 @@ default boolean onProduce(RelNode node) { input.setExplored(); for (RelSubset subset : input.getSet().subsets) { // Clear the LB cache as exploring state has changed. - input.getCluster().getMetadataQuery().clearCache(subset); + input.getCluster().getMetadataQuery().cache.clear(subset); Review comment: This feels like a step backwards. It forces a RelMetadataQuery implementation to have a public field called cache of a certain type that exposes a method. Can you please revert it? Cache concerns should be internal (a relmetadataquery implementation may or may not have a cache and external users like VolcanoPlanner only need to be able to clear rows). If you need access in janino code, please pass in as part of object initialization, lambda or a subclass that exposes the property. ########## File path: core/src/main/java/org/apache/calcite/rel/metadata/TableMetadataCache.java ########## @@ -0,0 +1,61 @@ +/* + * 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 com.google.common.collect.HashBasedTable; +import com.google.common.collect.Table; + +import org.checkerframework.checker.nullness.qual.Nullable; + +import java.util.Map; + +/** + * Rel Metadata cache back by @see HashBasedTable table. + */ +public class TableMetadataCache implements MetadataCache { Review comment: Let's move this to package protected visibility. It makes things clearer that this isn't supposed to be exposed as an api. ########## 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 think we should deprecate this method as part of this patch and remove any uses of it. ########## 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; Review comment: This is potentially a change in behavior. Since the map is still visible, people may be relying on it (filling or clearing). Keeping the field but ignoring it is not a safe change. I think you have to do this change over two releases. Deprecate it in one release then stop using it/remove it in the next release. ########## 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; Review comment: Note, that rather than exposing the cache as a public field to the world, I believe we can capture the same dynamic by simply passing a reference into the janino generated code (and anyone else who needs access to it). ########## File path: core/src/main/java/org/apache/calcite/rel/metadata/RelMetadataQueryBase.java ########## @@ -114,13 +119,8 @@ protected RelMetadataQueryBase(@Nullable JaninoRelMetadataProvider metadataProvi * @param rel RelNode whose cached metadata should be removed * @return true if cache for the provided RelNode was not empty */ + @Deprecated // to be removed before 2.0 Review comment: as mentioned elsewhere, please remove this deprecation. ########## 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: I agree with this general approach. The concepts of code generation and ReviseableHandlerProvider are two separate concepts. I would actually name this more to that point e.g. "ReviseableHandlerProvider". As part of this introduction, can we make we remove JaninoRelMetadataProvider since now we really only need a ReviseableMetadataProvider that uses the JaninoReviseableHandlerProvider? (by remove I mean remove all code, uses and deprecate. It would still exist for compatibility reasons) ########## File path: core/src/main/java/org/apache/calcite/rel/metadata/janino/CacheGenerator.java ########## @@ -0,0 +1,423 @@ +/* + * 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.janino; + +import org.apache.calcite.linq4j.Ord; +import org.apache.calcite.linq4j.tree.Primitive; +import org.apache.calcite.rel.RelNode; +import org.apache.calcite.rel.metadata.CyclicMetadataException; +import org.apache.calcite.rel.metadata.DelegatingMetadataRel; +import org.apache.calcite.rel.metadata.NullSentinel; +import org.apache.calcite.rel.metadata.RelMetadataQuery; +import org.apache.calcite.rex.RexNode; +import org.apache.calcite.runtime.FlatLists; + +import com.google.common.collect.ImmutableList; + +import java.lang.reflect.Method; + +import static org.apache.calcite.rel.metadata.janino.CodeGeneratorUtil.argList; +import static org.apache.calcite.rel.metadata.janino.CodeGeneratorUtil.paramList; + +/** + * Generates caching code for janino backed metadata. + */ +class CacheGenerator { Review comment: Looking at this further, I think the janino cache refactoring should be separated into a separate pr after we get the updated apis merged. Can you pull this out for now? ########## File path: core/src/main/java/org/apache/calcite/rel/metadata/janino/RelMetadataHandlerGenerator.java ########## @@ -0,0 +1,138 @@ +/* + * 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.janino; + +import org.apache.calcite.linq4j.Ord; +import org.apache.calcite.rel.metadata.MetadataDef; +import org.apache.calcite.rel.metadata.MetadataHandler; + +import org.checkerframework.checker.nullness.qual.Nullable; +import org.immutables.value.Value; + +import java.lang.reflect.Method; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; + +/** + * Generates the {@link MetadataHandler} code. Review comment: Per my other comments, can you please extract the janino code generation refactoring out of this patch and deal with it as a separate PR? Thanks ########## 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'm not excited about depending on another provider interface in RelMetadataQuery (and introducing another public api). Can we come up with a way to extract this out? (Separate the concept of handlers from the relmetadataquery surface area.) ########## 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: I'm against this change as it doesn't really reduce the required pattern/encapsulation between RelMetadataQuery and another provider concept. ########## File path: core/src/main/java/org/apache/calcite/rel/metadata/janino/CacheUtil.java ########## @@ -0,0 +1,42 @@ +/* + * 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.janino; + +import java.util.stream.IntStream; + +/** + * Functions used by generated code. + */ +public class CacheUtil { Review comment: Can we make this package visible? Note, if you're having visibility problems, you can control what package the janino code produces in to address. Alternatively, declare this as a interface and have the janino generated code that needs access to it extend this interface. ########## 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: This `MetadataProvider` interface is really "a handler way to deal with metadata that is no longer coupled to janino". I agree that this abstraction makes sense but it feels too specific to be added as a RelMetadataQuery dependency. I'd like to see it instead added to some separate impl specific version of relmetadataquery e.g. . My best thought currently is modifying RelMetadataQuery to be a decorator on top of a new "AbstractRelMetdataQuery". Then the handler concepts can be moved to an implementation of AbstractRelMetdataQuery and RelMetadataQuery doesn't depend on a new implementation specific interface. Concepts like null -> sentinel conversion and convenience delegations (getColumnOrigin -> getColumnOrigins) can be done at the RelMetadataQuery level but other concepts should be removed. -- 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]
