Rename and address comments

anlowee · anlowee · commit c302fbe16477 · 2025-08-14T19:37:26.000Z
diff --git a/presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpPlanOptimizer.java b/presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpPlanOptimizer.java
@@ -63,7 +63,7 @@ public PlanNode optimize(PlanNode maxSubplan, ConnectorSession session, Variable
         Rewriter rewriter = new Rewriter(idAllocator);
         PlanNode optimizedPlanNode = rewriteWith(rewriter, maxSubplan);
 
-        // Throw exception if there are any required metadata filters
+        // Throw exception if any required metadata filters are missing
         if (!rewriter.tableScopeSet.isEmpty() && !rewriter.hasVisitedFilter) {
             metadataFilterProvider.checkContainsRequiredFilters(rewriter.tableScopeSet, "");
         }
diff --git a/presto-clp/src/main/java/com/facebook/presto/plugin/clp/metadata/filter/ClpMetadataFilterConfig.java b/presto-clp/src/main/java/com/facebook/presto/plugin/clp/metadata/filter/ClpMetadataFilterConfig.java
@@ -29,17 +29,17 @@
  *   filter is missing or cannot be pushed down, the query will be rejected.</li>
  * </ul>
  */
-public class ClpMetadataFilter
+public class ClpMetadataFilterConfig
 {
     @JsonProperty("columnName")
     public String columnName;
 
     @JsonProperty("metadataProviderSpecific")
-    public MetadataProviderSpecific metadataProviderSpecific;
+    public MetadataProviderSpecificOptions metadataProviderSpecific;
 
     @JsonProperty("required")
     public boolean required;
 
-    public interface MetadataProviderSpecific
+    public interface MetadataProviderSpecificOptions
     {}
 }
diff --git a/presto-clp/src/main/java/com/facebook/presto/plugin/clp/metadata/filter/ClpMetadataFilterProvider.java b/presto-clp/src/main/java/com/facebook/presto/plugin/clp/metadata/filter/ClpMetadataFilterProvider.java
@@ -31,7 +31,7 @@
 
 import static com.facebook.presto.plugin.clp.ClpErrorCode.CLP_MANDATORY_METADATA_FILTER_NOT_VALID;
 import static com.facebook.presto.plugin.clp.ClpErrorCode.CLP_METADATA_FILTER_CONFIG_NOT_FOUND;
-import static com.facebook.presto.plugin.clp.metadata.filter.ClpMetadataFilter.MetadataProviderSpecific;
+import static com.facebook.presto.plugin.clp.metadata.filter.ClpMetadataFilterConfig.MetadataProviderSpecificOptions;
 import static com.google.common.collect.ImmutableSet.toImmutableSet;
 import static java.util.Objects.requireNonNull;
 
@@ -46,11 +46,11 @@
  * to all tables within that schema).
  * <p></p>
  * Different metadata providers can customize filter configurations through the
- * {@code metadataProviderSpecific} field within each {@link ClpMetadataFilter}.
+ * {@code metadataProviderSpecific} field within each {@link ClpMetadataFilterConfig}.
  */
 public abstract class ClpMetadataFilterProvider
 {
-    protected final Map<String, List<ClpMetadataFilter>> filterMap;
+    protected final Map<String, List<ClpMetadataFilterConfig>> filterMap;
 
     public ClpMetadataFilterProvider(ClpConfig config)
     {
@@ -63,22 +63,22 @@ public ClpMetadataFilterProvider(ClpConfig config)
         ObjectMapper mapper = new ObjectMapper();
         SimpleModule module = new SimpleModule();
         module.addDeserializer(
-                MetadataProviderSpecific.class,
-                new ClpMetadataProviderSpecificDeserializer(getMetadataProviderSpecificClass()));
+                MetadataProviderSpecificOptions.class,
+                new ClpMetadataProviderSpecificOptionsDeserializer(getMetadataProviderSpecificOptionsClass()));
         mapper.registerModule(module);
         try {
             filterMap = mapper.readValue(
                     new File(config.getMetadataFilterConfig()),
-                    new TypeReference<Map<String, List<ClpMetadataFilter>>>() {});
+                    new TypeReference<Map<String, List<ClpMetadataFilterConfig>>>() {});
         }
         catch (IOException e) {
             throw new PrestoException(CLP_METADATA_FILTER_CONFIG_NOT_FOUND, "Failed to open metadata filter config file", e);
         }
     }
 
     /**
-     * Rewrites the given pushed-down expression for metadata filtering to remap filter conditions
-     * based on the configured range mappings for the given scope.
+     * Rewrites the given {@code pushDownExpression} for metadata filtering to remap filter
+     * conditions based on the configured range mappings for the given scope.
      * <p></p>
      * The {@code scope} follows the format {@code catalog[.schema][.table]}, and determines
      * which filter mappings to apply, since mappings from more specific scopes (e.g., table-level)
@@ -87,19 +87,19 @@ public ClpMetadataFilterProvider(ClpConfig config)
      * filter configuration.
      *
      * @param scope the scope of the filter
-     * @param pushDownExpression the pushed-down expression for metadata filtering that needs to
-     *                           be rewritten
-     * @return the rewritten pushed-down expression for metadata filtering
+     * @param pushDownExpression the {@code pushDownExpression} for metadata filtering that needs
+     *                           to be rewritten
+     * @return the rewritten {@code pushDownExpression} for metadata filtering
      */
     public abstract String remapMetadataFilterPushDown(String scope, String pushDownExpression);
 
     /**
-     * Checks for the given table, if the given pushed-down expression for metadata filtering
+     * Checks for the given table, if the given {@code pushDownExpression} for metadata filtering
      * contains all required fields.
      *
      * @param tableScopeSet the set of scopes of the tables that are being queried
-     * @param metadataFilterPushDownExpression the pushed-down expression for metadata filtering
-     *                                         to be checked
+     * @param metadataFilterPushDownExpression the {@code pushDownExpression} for metadata
+     *                                         filtering to be checked
      */
     public void checkContainsRequiredFilters(Set<String> tableScopeSet, String metadataFilterPushDownExpression)
     {
@@ -126,20 +126,20 @@ public Set<String> getColumnNames(String scope)
     }
 
     /**
-     * Returns the {@link MetadataProviderSpecific} class implemented by the user. To respect our
+     * Returns the {@link MetadataProviderSpecificOptions} class implemented by the user. To respect our
      * code style, we recommend implementing a {@code protected static class} as an inner class
      * in the user-implemented {@link ClpMetadataFilterProvider} class.
      *
-     * @return the user-implemented {@link MetadataProviderSpecific} class.
+     * @return the user-implemented {@link MetadataProviderSpecificOptions} class.
      */
-    protected abstract Class<? extends MetadataProviderSpecific> getMetadataProviderSpecificClass();
+    protected abstract Class<? extends MetadataProviderSpecificOptions> getMetadataProviderSpecificOptionsClass();
 
     private Set<String> getRequiredColumnNames(String scope)
     {
         return collectColumnNamesFromScopes(scope, this::getRequiredColumnNamesFromFilters);
     }
 
-    private Set<String> collectColumnNamesFromScopes(String scope, Function<List<ClpMetadataFilter>, Set<String>> extractor)
+    private Set<String> collectColumnNamesFromScopes(String scope, Function<List<ClpMetadataFilterConfig>, Set<String>> extractor)
     {
         String[] splitScope = scope.split("\\.");
         ImmutableSet.Builder<String> builder = ImmutableSet.builder();
@@ -157,14 +157,14 @@ private Set<String> collectColumnNamesFromScopes(String scope, Function<List<Clp
         return builder.build();
     }
 
-    private Set<String> getAllColumnNamesFromFilters(List<ClpMetadataFilter> filters)
+    private Set<String> getAllColumnNamesFromFilters(List<ClpMetadataFilterConfig> filters)
     {
         return null != filters ? filters.stream()
                 .map(filter -> filter.columnName)
                 .collect(toImmutableSet()) : ImmutableSet.of();
     }
 
-    private Set<String> getRequiredColumnNamesFromFilters(List<ClpMetadataFilter> filters)
+    private Set<String> getRequiredColumnNamesFromFilters(List<ClpMetadataFilterConfig> filters)
     {
         return null != filters ? filters.stream()
                 .filter(filter -> filter.required)
diff --git a/presto-clp/src/main/java/com/facebook/presto/plugin/clp/metadata/filter/ClpMetadataProviderSpecificOptionsDeserializer.java b/presto-clp/src/main/java/com/facebook/presto/plugin/clp/metadata/filter/ClpMetadataProviderSpecificOptionsDeserializer.java
@@ -21,29 +21,29 @@
 
 import java.io.IOException;
 
-import static com.facebook.presto.plugin.clp.metadata.filter.ClpMetadataFilter.MetadataProviderSpecific;
+import static com.facebook.presto.plugin.clp.metadata.filter.ClpMetadataFilterConfig.MetadataProviderSpecificOptions;
 
 /**
- * Uses the given implementation of {@link MetadataProviderSpecific} to deserialize the
+ * Uses the given implementation of {@link MetadataProviderSpecificOptions} to deserialize the
  * {@code "metadataProviderSpecific"} field in the filter. The implementation is originally chosen
  * by the {@code clp.metadata-provider-type} config option.
  */
-public class ClpMetadataProviderSpecificDeserializer
-        extends JsonDeserializer<MetadataProviderSpecific>
+public class ClpMetadataProviderSpecificOptionsDeserializer
+        extends JsonDeserializer<MetadataProviderSpecificOptions>
 {
-    private final Class<? extends MetadataProviderSpecific> actualMetadataProviderSpecificClass;
+    private final Class<? extends MetadataProviderSpecificOptions> actualMetadataProviderSpecificOptionsClass;
 
-    public ClpMetadataProviderSpecificDeserializer(Class<? extends MetadataProviderSpecific> actualMetadataProviderSpecificClass)
+    public ClpMetadataProviderSpecificOptionsDeserializer(Class<? extends MetadataProviderSpecificOptions> actualMetadataProviderSpecificOptionsClass)
     {
-        this.actualMetadataProviderSpecificClass = actualMetadataProviderSpecificClass;
+        this.actualMetadataProviderSpecificOptionsClass = actualMetadataProviderSpecificOptionsClass;
     }
 
     @Override
-    public MetadataProviderSpecific deserialize(JsonParser p, DeserializationContext ctxt) throws IOException
+    public MetadataProviderSpecificOptions deserialize(JsonParser p, DeserializationContext ctxt) throws IOException
     {
         ObjectNode node = p.getCodec().readTree(p);
         ObjectMapper mapper = (ObjectMapper) p.getCodec();
 
-        return mapper.treeToValue(node, actualMetadataProviderSpecificClass);
+        return mapper.treeToValue(node, actualMetadataProviderSpecificOptionsClass);
     }
 }
diff --git a/presto-clp/src/main/java/com/facebook/presto/plugin/clp/metadata/filter/ClpMySqlMetadataFilterProvider.java b/presto-clp/src/main/java/com/facebook/presto/plugin/clp/metadata/filter/ClpMySqlMetadataFilterProvider.java
@@ -23,7 +23,7 @@
 import java.util.Map;
 import java.util.Objects;
 
-import static com.facebook.presto.plugin.clp.metadata.filter.ClpMetadataFilter.MetadataProviderSpecific;
+import static com.facebook.presto.plugin.clp.metadata.filter.ClpMetadataFilterConfig.MetadataProviderSpecificOptions;
 import static com.google.common.collect.ImmutableMap.toImmutableMap;
 import static java.lang.String.format;
 
@@ -42,7 +42,7 @@ public ClpMySqlMetadataFilterProvider(ClpConfig config)
 
     /**
      * This method performs regex-based replacements according to the {@code "rangeMapping"} field
-     * in {@link ClpMySqlMetadataProviderSpecific} to convert numeric filter expressions. For
+     * in {@link ClpMySqlMetadataProviderSpecificOptions} to convert numeric filter expressions. For
      * example:
      * <ul>
      *   <li>{@code "msg.timestamp" >= 1234} → {@code end_timestamp >= 1234}</li>
@@ -60,7 +60,7 @@ public String remapMetadataFilterPushDown(String scope, String pushDownExpressio
     {
         String[] splitScope = scope.split("\\.");
 
-        Map<String, ClpMySqlMetadataProviderSpecific.RangeMapping> mappings = new HashMap<>(getAllMappingsFromFilters(filterMap.get(splitScope[0])));
+        Map<String, ClpMySqlMetadataProviderSpecificOptions.RangeMapping> mappings = new HashMap<>(getAllMappingsFromFilters(filterMap.get(splitScope[0])));
 
         if (1 < splitScope.length) {
             mappings.putAll(getAllMappingsFromFilters(filterMap.get(splitScope[0] + "." + splitScope[1])));
@@ -71,9 +71,9 @@ public String remapMetadataFilterPushDown(String scope, String pushDownExpressio
         }
 
         String remappedSql = pushDownExpression;
-        for (Map.Entry<String, ClpMySqlMetadataProviderSpecific.RangeMapping> entry : mappings.entrySet()) {
+        for (Map.Entry<String, ClpMySqlMetadataProviderSpecificOptions.RangeMapping> entry : mappings.entrySet()) {
             String key = entry.getKey();
-            ClpMySqlMetadataProviderSpecific.RangeMapping value = entry.getValue();
+            ClpMySqlMetadataProviderSpecificOptions.RangeMapping value = entry.getValue();
             remappedSql = remappedSql.replaceAll(
                     format("\"(%s)\"\\s(>=?)\\s(-?[0-9]+(?:\\.[0-9]+)?(?:[eE][+-]?[0-9]+)?)", key),
                     format("%s $2 $3", value.upperBound));
@@ -88,21 +88,21 @@ public String remapMetadataFilterPushDown(String scope, String pushDownExpressio
     }
 
     @Override
-    protected Class<? extends MetadataProviderSpecific> getMetadataProviderSpecificClass()
+    protected Class<? extends MetadataProviderSpecificOptions> getMetadataProviderSpecificOptionsClass()
     {
-        return ClpMySqlMetadataProviderSpecific.class;
+        return ClpMySqlMetadataProviderSpecificOptions.class;
     }
 
-    private Map<String, ClpMySqlMetadataProviderSpecific.RangeMapping> getAllMappingsFromFilters(List<ClpMetadataFilter> filters)
+    private Map<String, ClpMySqlMetadataProviderSpecificOptions.RangeMapping> getAllMappingsFromFilters(List<ClpMetadataFilterConfig> filters)
     {
         return null != filters
                 ? filters.stream()
                 .filter(filter ->
-                        filter.metadataProviderSpecific instanceof ClpMySqlMetadataProviderSpecific &&
-                                ((ClpMySqlMetadataProviderSpecific) filter.metadataProviderSpecific).rangeMapping != null)
+                        filter.metadataProviderSpecific instanceof ClpMySqlMetadataProviderSpecificOptions &&
+                                ((ClpMySqlMetadataProviderSpecificOptions) filter.metadataProviderSpecific).rangeMapping != null)
                 .collect(toImmutableMap(
                         filter -> filter.columnName,
-                        filter -> ((ClpMySqlMetadataProviderSpecific) filter.metadataProviderSpecific).rangeMapping))
+                        filter -> ((ClpMySqlMetadataProviderSpecificOptions) filter.metadataProviderSpecific).rangeMapping))
                 : ImmutableMap.of();
     }
 
@@ -120,8 +120,8 @@ private Map<String, ClpMySqlMetadataProviderSpecific.RangeMapping> getAllMapping
      *   </li>
      * </ul>
      */
-    protected static class ClpMySqlMetadataProviderSpecific
-            implements MetadataProviderSpecific
+    protected static class ClpMySqlMetadataProviderSpecificOptions
+            implements MetadataProviderSpecificOptions
     {
         @JsonProperty("rangeMapping")
         public RangeMapping rangeMapping;
diff --git a/presto-docs/src/main/sphinx/connector/clp.rst b/presto-docs/src/main/sphinx/connector/clp.rst
@@ -58,7 +58,8 @@ Property Name                      Description
 ``clp.metadata-provider-type``     Specifies the metadata provider type. Currently, the only supported      ``mysql``
                                    type is a MySQL database, which is also used by the CLP package to store
                                    metadata. Additional providers can be supported by implementing the
-                                   ``ClpMetadataProvider`` interface.
+                                   ``ClpMetadataProvider`` interface. You may also need to implement the
+                                   ``ClpMetadataFilterProvider`` interface.
 ``clp.metadata-db-url``            The JDBC URL used to connect to the metadata database. This property is
                                    required if ``clp.metadata-provider-type`` is set to ``mysql``.
 ``clp.metadata-db-name``           The name of the metadata database. This option is required if
@@ -95,8 +96,8 @@ uses a MySQL database for both metadata and split storage. We recommend using th
 automatically populates the database with the required information.
 
 If you prefer to use a different source--or the same source with a custom implementation--you can provide your own
-implementations of the ``ClpMetadataProvider`` and ``ClpSplitProvider`` interfaces, and configure the connector
-accordingly.
+implementations of the ``ClpMetadataFilterProvider``, ``ClpMetadataProvider`` and ``ClpSplitProvider`` interfaces, and
+configure the connector accordingly.
 
 .. _metadata-filter-config-file:
 
@@ -143,6 +144,7 @@ Each filter config has the following options:
 
 - ``metadataProviderSpecific`` *(optional)*: Configs specific to the current metadata provider. Options for the default
   metadata provider (``ClpMySqlMetadataProvider``) are :ref:`below<clp-mysql-metadata-provider-specific-filter-config>`.
+  Customized
 
 - ``required`` *(optional, defaults to false)*: Whether the filter **must** be present in the generated metadata query.
   If a required filter is missing or cannot be added to the metadata query, the original query will be rejected.

Original file line number	Diff line number	Diff line change
`@@ -63,7 +63,7 @@ public PlanNode optimize(PlanNode maxSubplan, ConnectorSession session, Variable`
`63`	`63`	`Rewriter rewriter = new Rewriter(idAllocator);`
`64`	`64`	`PlanNode optimizedPlanNode = rewriteWith(rewriter, maxSubplan);`
`65`	`65`
`66`		`- // Throw exception if there are any required metadata filters`
	`66`	`+ // Throw exception if any required metadata filters are missing`
`67`	`67`	`if (!rewriter.tableScopeSet.isEmpty() && !rewriter.hasVisitedFilter) {`
`68`	`68`	`metadataFilterProvider.checkContainsRequiredFilters(rewriter.tableScopeSet, "");`
`69`	`69`	`}`
Original file line number	Diff line number	Diff line change
`@@ -21,29 +21,29 @@`
`21`	`21`
`22`	`22`	`import java.io.IOException;`
`23`	`23`
`24`		`-import static com.facebook.presto.plugin.clp.metadata.filter.ClpMetadataFilter.MetadataProviderSpecific;`
	`24`	`+import static com.facebook.presto.plugin.clp.metadata.filter.ClpMetadataFilterConfig.MetadataProviderSpecificOptions;`
`25`	`25`
`26`	`26`	`/**`
`27`		`- * Uses the given implementation of {@link MetadataProviderSpecific} to deserialize the`
	`27`	`+ * Uses the given implementation of {@link MetadataProviderSpecificOptions} to deserialize the`
`28`	`28`	`* {@code "metadataProviderSpecific"} field in the filter. The implementation is originally chosen`
`29`	`29`	`* by the {@code clp.metadata-provider-type} config option.`
`30`	`30`	`*/`
`31`		`-public class ClpMetadataProviderSpecificDeserializer`
`32`		`- extends JsonDeserializer<MetadataProviderSpecific>`
	`31`	`+public class ClpMetadataProviderSpecificOptionsDeserializer`
	`32`	`+ extends JsonDeserializer<MetadataProviderSpecificOptions>`
`33`	`33`	`{`
`34`		`- private final Class<? extends MetadataProviderSpecific> actualMetadataProviderSpecificClass;`
	`34`	`+ private final Class<? extends MetadataProviderSpecificOptions> actualMetadataProviderSpecificOptionsClass;`
`35`	`35`
`36`		`- public ClpMetadataProviderSpecificDeserializer(Class<? extends MetadataProviderSpecific> actualMetadataProviderSpecificClass)`
	`36`	`+ public ClpMetadataProviderSpecificOptionsDeserializer(Class<? extends MetadataProviderSpecificOptions> actualMetadataProviderSpecificOptionsClass)`
`37`	`37`	`{`
`38`		`- this.actualMetadataProviderSpecificClass = actualMetadataProviderSpecificClass;`
	`38`	`+ this.actualMetadataProviderSpecificOptionsClass = actualMetadataProviderSpecificOptionsClass;`
`39`	`39`	`}`
`40`	`40`
`41`	`41`	`@Override`
`42`		`- public MetadataProviderSpecific deserialize(JsonParser p, DeserializationContext ctxt) throws IOException`
	`42`	`+ public MetadataProviderSpecificOptions deserialize(JsonParser p, DeserializationContext ctxt) throws IOException`
`43`	`43`	`{`
`44`	`44`	`ObjectNode node = p.getCodec().readTree(p);`
`45`	`45`	`ObjectMapper mapper = (ObjectMapper) p.getCodec();`
`46`	`46`
`47`		`- return mapper.treeToValue(node, actualMetadataProviderSpecificClass);`
	`47`	`+ return mapper.treeToValue(node, actualMetadataProviderSpecificOptionsClass);`
`48`	`48`	`}`
`49`	`49`	`}`