Apply suggestions from code review

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

Author: anlowee

presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpExpression.java

@@ -18,17 +18,21 @@
 import java.util.Optional;
 
 /**
- * Represents the result of converting a Presto RowExpression into a CLP-compatible KQL query and
- * a SQL query for filtering splits using a metadata database. In every case, `pushDownExpression`
- * represents the part of the RowExpression that could be converted to a KQL expression, and
- * `remainingExpression` represents the part that could not be converted.
+ * Represents the result of:
+ * <ul>
+ *     <li>splitting a {@link RowExpression} into an expression that can be pushed down to CLP
+ *     (`pushDownExpression`) and any remaining expression that cannot be
+ *     (`remainingExpression`).</li>
+ *     <li>a SQL query for filtering splits using CLP's metadata database.</li>
+ * </ul>
  */
 public class ClpExpression
 {
     // Optional KQL query or column name representing the fully or partially translatable part of the expression.
     private final Optional<String> pushDownExpression;
 
-    // Optional SQL string extracted from the query plan, which is only made of given metadata columns.
+    // Optional SQL string extracted from the query plan, which is only made of up of columns in
+    // CLP's metadata database.
     private final Optional<String> metadataSqlQuery;
 
     // The remaining (non-translatable) portion of the RowExpression, if any.

presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpFilterToKqlConverter.java

@@ -84,8 +84,8 @@
  *     <li>Dereferencing fields from row-typed variables.</li>
  *     <li>Logical operators AND, OR, and NOT.</li>
  * </ul>
- *
- * Supported translations for Metadata SQL include:
+ * <p></p>
+ * Supported translations for SQL include:
  * <ul>
  *     <li>Comparisons between variables and constants (e.g., =, !=, <, >, <=, >=).</li>
  *     <li>Dereferencing fields from row-typed variables.</li>
@@ -209,7 +209,7 @@ private String getVariableName(VariableReferenceExpression variable)
      * Handles the <code>BETWEEN</code> expression.
      * <p></p>
      * The translation is only performed if:
-     * <ul>All arguments must have numeric types.</ul>
+     * <ul>All arguments have numeric types.</ul>
      * <ul>The first argument is a variable reference expression.</ul>
      * <ul>The second and third arguments are constant expressions.</ul>
      * <p></p>
@@ -866,12 +866,12 @@ private ClpExpression handleDereference(RowExpression expression)
     }
 
     /**
-     * Refer to
+     * See
      * <a href="https://docs.yscope.com/clp/main/user-guide/reference-json-search-syntax">here
      * </a> for all special chars in the string value that need to be escaped.
      *
-     * @param literalString the target string to escape special chars '\', '"', '?' and '*'
-     * @return the escaped string
+     * @param literalString
+     * @return the string with special characters escaped
      */
     public static String escapeKqlSpecialCharsForStringValue(String literalString)
     {
@@ -884,11 +884,11 @@ public static String escapeKqlSpecialCharsForStringValue(String literalString)
     }
 
     /**
-     * Check if the type is one of the numeric types that CLP will handle. Refer to
-     * {@link ClpSchemaTree} for all types that CLP will handle.
+     * Checks if the type is one of the numeric types that can be pushed down to CLP. Refer to
+     * {@link ClpSchemaTree} for all types that can be pushed down.
      *
      * @param type the type to check
-     * @return is the type numeric or not
+     * @return whether the type can be pushed down.
      */
     public static boolean isNumericType(Type type)
     {

presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpMetadataFilterProvider.java

@@ -42,20 +42,18 @@
 
 /**
  * Loads and manages metadata filter configurations for the CLP connector.
- * <p>
+ * <p></p>
  * The configuration file is specified by the {@code clp.metadata-filter-config} property
  * and defines metadata filters used to optimize query execution through split pruning.
- * Filters can be declared at different scopes:
+ * <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>
+ * Each filter config includes the following fields:
  * <ul>
- *   <li><b>Catalog-level</b>: applies to all schemas and tables within a catalog.</li>
- *   <li><b>Schema-level</b>: applies to all tables within a specific catalog and schema.</li>
- *   <li><b>Table-level</b>: applies to a fully qualified table {@code catalog.schema.table}.</li>
- * </ul>
- *
- * <p>Each scope maps to a list of filter definitions. Each filter includes the following fields:
- * <ul>
- *   <li><b>{@code columnName}</b> (required): the name of a column in the table's logical schema.
- *       Only columns of numeric type are currently supported as metadata filters.</li>
+ *   <li><b>{@code columnName}</b>: the name of a column in the table's logical schema. Currently,
+ *   only numeric-type columns can be used as metadata filters.</li>
  *
  *   <li><b>{@code rangeMapping}</b> (optional): remaps a logical filter to physical metadata-only columns.
  *       This field is valid only for numeric-type columns.
@@ -69,9 +67,9 @@
  *       }</pre>
  *       This ensures the filter applies to a superset of the actual result set, enabling safe pruning.</li>
  *
- *   <li><b>{@code required}</b> (optional, default: {@code false}): indicates whether the filter must be present
- *       in the extracted metadata filter SQL query. 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}): indicates whether the filter
+ *   must be present in the translated metadata filter SQL query. If a required filter is missing or
+ *   cannot be pushed down, the query will be rejected.</li>
  * </ul>
  */
 public class ClpMetadataFilterProvider
@@ -112,29 +110,27 @@ public void checkContainsRequiredFilters(SchemaTableName schemaTableName, String
     }
 
     /**
-     * Rewrites the input SQL string by remapping filter conditions based on the configured
-     * metadata filter range mappings for the given scope.
+     * Rewrites the given SQL string to remap filter conditions based on the configured range
+     * mappings for the given scope.
      *
      * <p>The {@code scope} follows the format {@code catalog[.schema][.table]}, and determines
-     * which filter mappings to apply. For each level of scope (catalog, schema, table), this
-     * method collects all range mappings defined in the metadata filter configuration. Mappings
-     * from more specific scopes (e.g., table-level) override or supplement those from broader
-     * scopes (e.g., catalog-level).
+     * 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 metadata
+     * filter configuration.
      *
      * <p>This method performs regex-based replacements to convert numeric filter expressions such
      * as:
-     *
      * <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 catalog.schema.table scope used to resolve applicable filter mappings
-     * @param sql the original SQL expression to be remapped
-     * @return the rewritten SQL string with metadata filter expressions remapped according to the
-     * configured range mappings
+     * @param scope
+     * @param sql
+     * @return the rewritten SQL string
      */
     public String remapFilterSql(String scope, String sql)
     {

presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpFilterToKql.java

@@ -105,7 +105,9 @@ public void testBetweenPushDown()
                 "fare BETWEEN (city.Region.Id - 5) AND (city.Region.Id + 5)",
                 null,
                 "fare BETWEEN (city.Region.Id - 5) AND (city.Region.Id + 5)");
-        // If the last two arguments of BETWEEN are not numeric constants, then CLP connector won't do push down
+
+        // If the last two arguments of BETWEEN are not numeric constants, then the CLP connector
+        // won't push them down.
         testPushDown(sessionHolder, "city.Name BETWEEN 'a' AND 'b'", null, "city.Name BETWEEN 'a' AND 'b'");
     }