Apply suggestions from code review

Co-authored-by: kirkrodrigues <[email protected]>

Author: anlowee

presto-clp/src/main/java/com/facebook/presto/plugin/clp/split/filter/ClpCustomSplitFilterOptionsDeserializer.java

@@ -24,9 +24,9 @@
 import static com.facebook.presto.plugin.clp.split.filter.ClpSplitFilterConfig.CustomSplitFilterOptions;
 
 /**
- * Uses the given implementation of {@link CustomSplitFilterOptions} to deserialize the
- * {@code "customOptions"} field in the filter. The implementation is originally chosen
- * by the {@code clp.split-filter-provider-type} config option.
+ * Uses the given {@link CustomSplitFilterOptions} implementation to deserialize the
+ * {@code "customOptions"} field in a {@link ClpSplitFilterConfig}. The implementation is determined
+ * by the implemented {@link ClpSplitFilterProvider}.
  */
 public class ClpCustomSplitFilterOptionsDeserializer
         extends JsonDeserializer<CustomSplitFilterOptions>

presto-clp/src/main/java/com/facebook/presto/plugin/clp/split/filter/ClpMySqlSplitFilterProvider.java

@@ -28,8 +28,7 @@
 import static java.lang.String.format;
 
 /**
- * Loads and manages split filter configurations of MySQL metadata database for the CLP
- * connector.
+ * Implementation for the CLP package's MySQL metadata database.
  */
 public class ClpMySqlSplitFilterProvider
         extends ClpSplitFilterProvider
@@ -41,19 +40,18 @@ public ClpMySqlSplitFilterProvider(ClpConfig config)
     }
 
     /**
-     * This method performs regex-based replacements according to the {@code "rangeMapping"} field
-     * in {@link ClpMySqlCustomSplitFilterOptions} to convert numeric filter expressions. For
-     * example:
+     * Performs regex-based replacements to rewrite {@code pushDownExpression} according to the
+     * {@code "rangeMapping"} field in {@link ClpMySqlCustomSplitFilterOptions}. For example:
      * <ul>
      *   <li>{@code "msg.timestamp" >= 1234} → {@code end_timestamp >= 1234}</li>
      *   <li>{@code "msg.timestamp" <= 5678} → {@code begin_timestamp <= 5678}</li>
      *   <li>{@code "msg.timestamp" = 4567} →
      *   {@code (begin_timestamp <= 4567 AND end_timestamp >= 4567)}</li>
      * </ul>
      *
-     * @param scope the scope of the filter
-     * @param pushDownExpression the MySQL SQL string that needs to be rewritten
-     * @return the rewritten SQL string
+     * @param scope the filter's scope
+     * @param pushDownExpression the expression to be rewritten
+     * @return the rewritten expression
      */
     @Override
     public String remapSplitFilterPushDownExpression(String scope, String pushDownExpression)
@@ -107,15 +105,14 @@ private Map<String, ClpMySqlCustomSplitFilterOptions.RangeMapping> getAllMapping
     }
 
     /**
-     * The MySql-specific fields in the filter contains:
+     * Custom options:
      * <ul>
-     *   <li><b>{@code rangeMapping}</b> <i>(optional)</i>: an object only for numeric type filter
-     *   with the following properties:
+     *   <li><b>{@code rangeMapping}</b> <i>(optional)</i>: an object with the following properties:
      *      <ul>
-     *          <li>{@code lowerBound}: The metadata column that represents the lower bound of values
-     *          in a split for the data column.</li>
-     *          <li>{@code upperBound}: The metadata column that represents the upper bound of values
-     *          in a split for the data column.</li>
+     *          <li>{@code lowerBound}: The numeric metadata column that represents the lower bound
+     *          of values in a split for the numeric data column.</li>
+     *          <li>{@code upperBound}: The numeric metadata column that represents the upper bound
+     *          of values in a split for the numeric data column.</li>
      *      </ul>
      *   </li>
      * </ul>

presto-clp/src/main/java/com/facebook/presto/plugin/clp/split/filter/ClpSplitFilterConfig.java

@@ -16,17 +16,17 @@
 import com.fasterxml.jackson.annotation.JsonProperty;
 
 /**
- * Defines the basic filter JSON structure.
- * <p></p>
- * Here are the explanations for each field:
+ * Options for a how a column in a Presto query should be pushed down into a query against CLP's
+ * metadata database (during split pruning):
  * <ul>
- *   <li><b>{@code columnName}</b>: the data column's name.</li>
+ *   <li><b>{@code columnName}</b>: The column's name in the Presto query.</li>
  *
- *   <li><b>{@code customOptions}</b>: the split filter provider specific sub-object.</li>
+ *   <li><b>{@code customOptions}</b>: Options specific to the current
+ *   {@link ClpSplitFilterProvider}.</li>
  *
- *   <li><b>{@code required}</b> (optional, defaults to {@code false}): indicates whether the
- *   filter must be present in the pushed-down expression for split filtering. If a required
- *   filter is missing or cannot be pushed down, the query will be rejected.</li>
+ *   <li><b>{@code required}</b> (optional, defaults to {@code 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.</li>
  * </ul>
  */
 public class ClpSplitFilterConfig

presto-clp/src/main/java/com/facebook/presto/plugin/clp/split/filter/ClpSplitFilterProvider.java

@@ -36,17 +36,16 @@
 import static java.util.Objects.requireNonNull;
 
 /**
- * Loads and manages split filter configurations for the CLP connector.
+ * Loads and manages {@link ClpSplitFilterConfig}s from a config file.
  * <p></p>
- * The configuration file is specified by the {@code clp.split-filter-config} property and defines
- * split filters used to optimize query execution through split filtering.
+ * The config file is specified by the {@code clp.split-filter-config} property.
  * <p></p>
  * Filter configs can be declared at either a catalog, schema, or table scope. Filter configs under
  * a particular scope will apply to all child scopes (e.g., schema-level filter configs will apply
  * to all tables within that schema).
  * <p></p>
- * Different split filter providers can customize filter configurations through the
- * {@code "customOptions"} field within each {@link ClpSplitFilterConfig}.
+ * Implementations of this class can customize filter configs through the {@code "customOptions"}
+ * field within each {@link ClpSplitFilterConfig}.
  */
 public abstract class ClpSplitFilterProvider
 {
@@ -77,30 +76,27 @@ public ClpSplitFilterProvider(ClpConfig config)
     }
 
     /**
-     * Rewrites the given {@code pushDownExpression} for split filtering to remap filter conditions
-     * based on the custom configuration options stored defined in {@code "customOptions"} fields
-     * of the given scope.
+     * Rewrites {@code pushDownExpression} to remap filter conditions based on the
+     * {@code "customOptions"} 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)
+     * {@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)
      * override or supplement those from broader scopes (e.g., catalog-level). For each scope
-     * (catalog, schema, table), this method collects all range mappings defined in the split
-     * filter configuration.
+     * (catalog, schema, table), this method collects all mappings defined in
+     * {@code "customOptions"}.
      *
      * @param scope the scope of the filter
-     * @param pushDownExpression the {@code pushDownExpression} for split filtering that needs to
-     *                           be rewritten
-     * @return the rewritten {@code pushDownExpression} for split filtering
+     * @param pushDownExpression the expression to be rewritten
+     * @return the rewritten expression
      */
     public abstract String remapSplitFilterPushDownExpression(String scope, String pushDownExpression);
 
     /**
-     * Checks for the given table, if the given {@code pushDownExpression} for split filtering
-     * contains all required fields.
+     * Checks for the given table, if {@code splitFilterPushDownExpression} contains all required
+     * fields.
      *
      * @param tableScopeSet the set of scopes of the tables that are being queried
-     * @param splitFilterPushDownExpression the remapped {@code pushDownExpression} for split
-     *                                      filtering to be checked
+     * @param splitFilterPushDownExpression the expression to be checked
      */
     public void checkContainsRequiredFilters(Set<String> tableScopeSet, String splitFilterPushDownExpression)
     {
@@ -127,11 +123,11 @@ public Set<String> getColumnNames(String scope)
     }
 
     /**
-     * Returns the {@link CustomSplitFilterOptions} 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 ClpSplitFilterProvider} class.
+     * Returns the implemented {@link CustomSplitFilterOptions} class. To respect our code style, we
+     * recommend implementing a {@code protected static class} as an inner class in the implemented
+     * {@link ClpSplitFilterProvider} class.
      *
-     * @return the user-implemented {@link CustomSplitFilterOptions} class.
+     * @return the implemented {@link CustomSplitFilterOptions} class
      */
     protected abstract Class<? extends CustomSplitFilterOptions> getCustomSplitFilterOptionsClass();
 

presto-docs/src/main/sphinx/connector/clp.rst

@@ -79,8 +79,8 @@ Property Name                        Description
                                      seconds. Set this to a lower value for frequently changing datasets or
                                      to a higher value to reduce load.
 ``clp.split-filter-config``          The absolute path to an optional split filter config file. See the
-                                     :ref:`Split Filter Config File<split-filter-config-file>` section
-                                     for details.
+                                     :ref:`Split Filter Config File<split-filter-config-file>` section for
+                                     details.
 ``clp.split-filter-provider-type``   Specifies the split filter 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
@@ -146,20 +146,18 @@ Each filter config has the following options:
 
 - ``columnName``: The data column's name.
 
-- ``customOptions`` *(optional)*: Custom config options for a split filter provider. Options for the default split
-  filter provider (``ClpMySqlSplitFilterProvider``) are :ref:`below<clp-mysql-split-filter-provider-config>`.
+- ``customOptions`` *(optional)*: Custom options for a split filter provider. Options for the default split filter
+  provider (``ClpMySqlSplitFilterProvider``) are :ref:`below<clp-mysql-split-filter-provider-config>`.
 
-- ``required`` *(optional, defaults to false)*: Whether the filter **must** be present in the generated split filter
-  query. If a required filter is missing or cannot be added to the split filter query, the original query will be
-  rejected.
+- ``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.
 
 .. _clp-mysql-split-filter-provider-config:
 
 ClpMySqlSplitFilterProvider-Specific Filter Config
 -----------------------------------------------
 
-For the ``ClpMySqlSplitFilterProvider``, the ``customOptions`` option of the filter config has the following
-sub-options:
+For ``ClpMySqlSplitFilterProvider``, the ``customOptions`` option of the filter config has the following sub-options:
 
 - ``rangeMapping`` *(optional)*: an object with the following properties:
 
@@ -171,7 +169,7 @@ sub-options:
 Filter Config Example
 ---------------------
 
-The code block shows an example split filter config file:
+The code block shows an example filter config file:
 
 .. code-block:: json
 
@@ -217,7 +215,7 @@ The code block shows an example split filter config file:
     ``end_timestamp``, and is required to exist in every query.
   - The column ``file_name`` is used as-is without remapping.
 
-If you prefer to use a different format for the ``customOptions``, you can provide your own implementations of the
+If you prefer to use a different format for ``customOptions``, you can provide your own implementation of the
 ``ClpSplitFilterProvider`` interface, and configure the connector accordingly.
 
 Supported SQL Expressions