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.
--
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]
