[GitHub] [calcite] jacques-n commented on a change in pull request #2378: [CALCITE-4539] Customization of Metadata Handlers

[GitBox <gi...@apache.org>]

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: commits-unsubscribe@calcite.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org