[GitHub] [calcite] jacques-n commented on a change in pull request #2638: [CALCITE-4928] Decouple JaninoRelMetadataProvider from RelMetadataQuery

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

jacques-n commented on a change in pull request #2638:
URL: https://github.com/apache/calcite/pull/2638#discussion_r766840019



##########
File path: core/src/main/java/org/apache/calcite/rel/metadata/RelMetadataQueryBase.java
##########
@@ -67,6 +67,9 @@
   /** Set of active metadata queries, and cache of previous results. */
   public final Table<RelNode, Object, Object> map = HashBasedTable.create();
 
+  protected final @Nullable MetadataHandlerProvider provider;

Review comment:
       Updated

##########
File path: core/src/main/java/org/apache/calcite/rel/metadata/RelMetadataQueryBase.java
##########
@@ -75,11 +78,20 @@
       new ThreadLocal<>();
 
   //~ Constructors -----------------------------------------------------------
-
+  @Deprecated // to be removed before 2.0
   protected RelMetadataQueryBase(@Nullable JaninoRelMetadataProvider metadataProvider) {

Review comment:
       It is a public interface (public class, protected signature). While I agree that it would be unusual for a downstream project to call this, I don't think we should break public interfaces unless it is absolutely necessary.

##########
File path: core/src/main/java/org/apache/calcite/rel/metadata/RelMetadataQuery.java
##########
@@ -145,9 +184,10 @@ private RelMetadataQuery(@SuppressWarnings("unused") boolean dummy) {
     this.lowerBoundCostHandler = initialHandler(BuiltInMetadata.LowerBoundCost.Handler.class);
   }
 
-  private RelMetadataQuery(JaninoRelMetadataProvider metadataProvider,
+  private RelMetadataQuery(
+      @Nullable MetadataHandlerProvider metadataHandlerProvider,

Review comment:
       Good catch. It was due to an earlier version of the refactor. Removed.

##########
File path: core/src/main/java/org/apache/calcite/rel/metadata/RelMetadataQueryBase.java
##########
@@ -75,11 +78,20 @@
       new ThreadLocal<>();
 
   //~ Constructors -----------------------------------------------------------
-
+  @Deprecated // to be removed before 2.0
   protected RelMetadataQueryBase(@Nullable JaninoRelMetadataProvider metadataProvider) {
-    this.metadataProvider = metadataProvider;
+    this((MetadataHandlerProvider) metadataProvider);
+  }
+
+  @SuppressWarnings("deprecation")
+  protected RelMetadataQueryBase(@Nullable MetadataHandlerProvider provider) {
+    this.provider = provider;
+    this.metadataProvider = provider instanceof JaninoRelMetadataProvider
+        ? (JaninoRelMetadataProvider) provider : null;
   }
 
+  @SuppressWarnings("deprecation")

Review comment:
       You're right. There is a reference in this code to a deprecated class which I expected to trigger a warning.




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