jacques-n commented on a change in pull request #2378:
URL: https://github.com/apache/calcite/pull/2378#discussion_r742179082
##########
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'd like to separate the concern and the two solution into two separate
discussions.
First the concern:
The problem is the coupling between RelMetdataQuery and RelMetdataProvider.
In this patch you add additional new public apis that reinforce this coupling.
There is no need for the coupling. It is completely reasonable that someone
should be able to implement RelMetadataQuery without having to work at all with
RelMetdataProvider related items. That is the beauty of the RelMetadataQuery
surface area from the planning/rule pov. For example, someone should be able
hand write a RelMetadatQuery implementation that has all the handling of
supported rel types.
Second, the solution:
It is important to remove the coupling of RelMetdataQuery and
RelMetdataProvider. Ideally, we would convert RelMetadataQuery to an abstract
implementation (or interface) that only does a minimal number of data
cleansing/canonicalization options (null to sentinels, conveniences interfaces,
etc). However, because of the exposure of the protected constructor for
RelMetadatQuery, this has to be done in two steps to avoid breaking changes. I
proposed one way to get there but I don't think it is the only way. I don't
understand any of your comments around complexity, more subclasses, caching
cyclic dependency and boiler plate. As part of the first step towards a clean
api there has to be some shims to support old users until we can remove old
apis but that's also true with all of these metadata patches. We shouldn't
dismiss an ultimately cleaner solution because we have to maintain a shim for
one release (and really, a shim must exist in either case).
Lastly, it would be good to clarify what you mean by breaking changes. In
both the proposal I've put forward and the current patch, I see
deprecation/upgrade paths required (new apis that replace old apis exist
simultaneously and then old apis are removed post version deprecation). In both
cases, a user should have to do no code changes when upgrading but should see
deprecation warnings and then the following upgrade would see breakage if they
didn't solve deprecation warnings.
##########
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'd like to separate the concern and the solution into two separate
discussions.
First the concern:
The problem is the coupling between RelMetdataQuery and RelMetdataProvider.
In this patch you add additional new public apis that reinforce this coupling.
There is no need for the coupling. It is completely reasonable that someone
should be able to implement RelMetadataQuery without having to work at all with
RelMetdataProvider related items. That is the beauty of the RelMetadataQuery
surface area from the planning/rule pov. For example, someone should be able
hand write a RelMetadatQuery implementation that has all the handling of
supported rel types.
Second, the solution:
It is important to remove the coupling of RelMetdataQuery and
RelMetdataProvider. Ideally, we would convert RelMetadataQuery to an abstract
implementation (or interface) that only does a minimal number of data
cleansing/canonicalization options (null to sentinels, conveniences interfaces,
etc). However, because of the exposure of the protected constructor for
RelMetadatQuery, this has to be done in two steps to avoid breaking changes. I
proposed one way to get there but I don't think it is the only way. I don't
understand any of your comments around complexity, more subclasses, caching
cyclic dependency and boiler plate. As part of the first step towards a clean
api there has to be some shims to support old users until we can remove old
apis but that's also true with all of these metadata patches. We shouldn't
dismiss an ultimately cleaner solution because we have to maintain a shim for
one release (and really, a shim must exist in either case).
Lastly, it would be good to clarify what you mean by breaking changes. In
both the proposal I've put forward and the current patch, I see
deprecation/upgrade paths required (new apis that replace old apis exist
simultaneously and then old apis are removed post version deprecation). In both
cases, a user should have to do no code changes when upgrading but should see
deprecation warnings and then the following upgrade would see breakage if they
didn't solve deprecation warnings.
##########
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:
To put this another way: RelMetadataQuery is the what,
RelMetadataProvider is one possible how. Let's not couple the two.
##########
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).
##########
File path:
core/src/main/java/org/apache/calcite/rel/metadata/RelMetadataQuery.java
##########
@@ -886,10 +919,59 @@ public Boolean isVisibleInExplain(RelNode rel,
for (;;) {
try {
return lowerBoundCostHandler.getLowerBoundCost(rel, this, planner);
- } catch (JaninoRelMetadataProvider.NoHandler e) {
+ } catch (MetadataHandlerProvider.NoHandler e) {
lowerBoundCostHandler =
- revise(BuiltInMetadata.LowerBoundCost.Handler.class);
+
metadataHandlerProvider.revise(BuiltInMetadata.LowerBoundCost.Handler.class);
}
}
}
+
+ public static Builder<RelMetadataQuery> builder() {
Review comment:
If we want to go with a builder, I think we should make a couple
changes. (Not entirely sure a builder is yet necessary.)
-This should probably be a method called `supplierBuilder()` or similar
since it doesn't actually build the type of class it is contained within. I'd
expect a builder on RelMetdataQuery to build a `RelMetadataQuery`, not
`Supplier<RelMetadataQuery>`.
- I think that we may want to add other ways of building in the future. For
example I'm exploring the introduction of a lambda based metadata handling
systems. In that situation, we may want to construct one of those using the
same initial builder. As such I suggest a specialization strategy where calling
withProviders returns a subtype of builder that has configuration options
related to a provider based builder. Maybe in the future we have a separate one
that works based on lambdas and we call withLambdas() to use it (and a call to
that would specialize the builder to that type of metadataquery supplier.
##########
File path:
core/src/main/java/org/apache/calcite/rel/metadata/RelMetadataQuery.java
##########
@@ -886,10 +919,59 @@ public Boolean isVisibleInExplain(RelNode rel,
for (;;) {
try {
return lowerBoundCostHandler.getLowerBoundCost(rel, this, planner);
- } catch (JaninoRelMetadataProvider.NoHandler e) {
+ } catch (MetadataHandlerProvider.NoHandler e) {
lowerBoundCostHandler =
- revise(BuiltInMetadata.LowerBoundCost.Handler.class);
+
metadataHandlerProvider.revise(BuiltInMetadata.LowerBoundCost.Handler.class);
}
}
}
+
+ public static Builder<RelMetadataQuery> builder() {
Review comment:
If we want to go with a builder, I think we should make a couple
changes. (Not entirely sure a builder is yet necessary.)
- This should probably be a method called `supplierBuilder()` or similar
since it doesn't actually build the type of class it is contained within. I'd
expect a builder on RelMetdataQuery to build a `RelMetadataQuery`, not
`Supplier<RelMetadataQuery>`.
- I think that we may want to add other ways of building in the future. For
example I'm exploring the introduction of a lambda based metadata handling
systems. In that situation, we may want to construct one of those using the
same initial builder. As such I suggest a specialization strategy where calling
withProviders returns a subtype of builder that has configuration options
related to a provider based builder. Maybe in the future we have a separate one
that works based on lambdas and we call withLambdas() to use it (and a call to
that would specialize the builder to that type of metadataquery supplier.
##########
File path:
core/src/main/java/org/apache/calcite/rel/metadata/RelMetadataQuery.java
##########
@@ -886,10 +919,59 @@ public Boolean isVisibleInExplain(RelNode rel,
for (;;) {
try {
return lowerBoundCostHandler.getLowerBoundCost(rel, this, planner);
- } catch (JaninoRelMetadataProvider.NoHandler e) {
+ } catch (MetadataHandlerProvider.NoHandler e) {
lowerBoundCostHandler =
- revise(BuiltInMetadata.LowerBoundCost.Handler.class);
+
metadataHandlerProvider.revise(BuiltInMetadata.LowerBoundCost.Handler.class);
}
}
}
+
+ public static Builder<RelMetadataQuery> builder() {
Review comment:
If we want to go with a builder, I think we should make a couple
changes. (Not entirely sure a builder is yet necessary.)
- This should probably be a method called `supplierBuilder()` or similar
since it doesn't actually build the type of class it is contained within. I'd
expect `RelMetdataQuery.builder()` to build a `RelMetadataQuery`, not
`Supplier<RelMetadataQuery>`.
- I think that we may want to add other ways of building in the future. For
example I'm exploring the introduction of a lambda based metadata handling
systems. In that situation, we may want to construct one of those using the
same initial builder. As such I suggest a specialization strategy where calling
withProviders returns a subtype of builder that has configuration options
related to a provider based builder. Maybe in the future we have a separate one
that works based on lambdas and we call withLambdas() to use it (and a call to
that would specialize the builder to that type of metadataquery supplier.
--
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]
