jamesstarr commented on a change in pull request #2378:
URL: https://github.com/apache/calcite/pull/2378#discussion_r739618684
##########
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:
At this point the MetadataProvider a list a of tuples (Class<? extends
MetadataHandler>, MetadataHandler instance) which represent binding a set
methods to a particular metadata. Without this patch the Metadata class is not
used in any meaningful way in code path that is not already deprecated. You
have to have a symbol(global constant) to bind a the metadata implementation
too.
Part of the whole point of the interface is to let people customize/change
the behavior of the handler generation. Some people want to eagerly generate
the code at runtime, other want to tweaks the caching of generated classes, you
and others do not want to use janino altogether. This would support all of
those use cases.
RelMetadataQuery needs to call the implementation of the metadata calls
which is currently done through handlers. RelMetadataQueryBase already has a
supported api for doing conceptually similar things with similar amount of
exposed api. Their are just a set of things that you can not do with that
existing and supported API that I am attempting to rectify.
Several consumer of apache calcite have custom metadata calls. Currently
the suggested way to create a custom call is to subclass RelMetadataQuery.
What you are purposing would require 2 different subclasses of RelMetadataQuery
and also require them to correctly maintain the caching/cyclic dependency
boiler plate. I understand the desire to be able to debug code and have a
separation of concerns but the API you are proposing would require several
subclasses to do something conceptually simple and introduce breaking changes.
--
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]
