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

@@ -64,13 +64,16 @@
 import static java.util.Objects.requireNonNull;
 
 /**
- * A translator to translate Presto RowExpressions into KQL (Kibana Query Language) filters used as
- * CLP queries and SQL filters used for filtering splits by metadata database. This is used
- * primarily for pushing down supported filters to the CLP engine. This class implements the
- * {@link RowExpressionVisitor} interface and recursively walks Presto filter expressions,
- * attempting to convert supported expressions into corresponding KQL filter strings and SQL
- * filter strings for metadata filtering. Any part of the expression that cannot be translated to
- * KQL is preserved as a "remaining expression" for potential fallback processing.
+ * A translator to translate Presto {@link RowExpression}s into:
+ * <ul>
+ *     <li>KQL (Kibana Query Language) filters used to push down supported filters to the CLP
+ *     engine.</li>
+ *     <li>SQL filters used for filtering splits in CLP's metadata database.</li>
+ * </ul>
+ * This class implements the {@link RowExpressionVisitor} interface and recursively walks Presto
+ * filter expressions, attempting to convert supported expressions into corresponding KQL filter
+ * strings and SQL filter strings for metadata filtering. Any part of the expression that cannot be
+ * translated to KQL is preserved as a "remaining expression" for potential fallback processing.
  * <p></p>
  * Supported translations for KQL include:
  * <ul>
@@ -84,8 +87,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,9 +212,11 @@ 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>The first argument is a variable reference expression.</ul>
-     * <ul>The second and third arguments are constant expressions.</ul>
+     * <ul>
+     *     <li>all arguments have numeric types.</li>
+     *     <li>the first argument is a variable reference expression.</li>
+     *     <li>the second and third arguments are constant expressions.</li>
+     * </ul>
      * <p></p>
      * Example: <code>col1 BETWEEN 0 AND 5</code> → <code>col1 >= 0 AND col1 <= 5</code>
      *
@@ -234,9 +239,9 @@ private ClpExpression handleBetween(CallExpression node)
                 || !(third instanceof ConstantExpression)) {
             return new ClpExpression(node);
         }
-        if (!isNumericType(first.getType())
-                || !isNumericType(second.getType())
-                || !isNumericType(third.getType())) {
+        if (!isClpCompatibleNumericType(first.getType())
+                || !isClpCompatibleNumericType(second.getType())
+                || !isClpCompatibleNumericType(third.getType())) {
             return new ClpExpression(node);
         }
         Optional<String> variableOpt = first.accept(this, null).getPushDownExpression();
@@ -866,12 +871,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,13 +889,12 @@ 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.
      *
      * @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)
+    public static boolean isClpCompatibleNumericType(Type type)
     {
         return type.equals(BIGINT)
                 || type.equals(INTEGER)

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

@@ -42,36 +42,54 @@
 
 /**
  * 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>
+ *   <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>
  *
- * <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 rangeMapping}</b> <i>(optional)</i>: an object with the following properties:
+ *
+ *      <br><br>
+ *      <b>Note:</b> This option is only valid if the column has a numeric type.
+ *
+ *      <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>
+ *      </ul>
  *
- *   <li><b>{@code rangeMapping}</b> (optional): remaps a logical filter to physical metadata-only columns.
- *       This field is valid only for numeric-type columns.
- *       For example, a condition such as:
- *       <pre>{@code
- *       "msg.timestamp" > 1234 AND "msg.timestamp" < 5678
- *       }</pre>
- *       will be rewritten as:
- *       <pre>{@code
- *       "end_timestamp" > 1234 AND "begin_timestamp" < 5678
- *       }</pre>
- *       This ensures the filter applies to a superset of the actual result set, enabling safe pruning.</li>
+ *      <p>
+ *          For example, a condition such as:
+ *      </p>
+ *      <pre>{@code
+ *          "msg.timestamp" > 1234 AND "msg.timestamp" < 5678
+ *      }</pre>
  *
- *   <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>
+ *      <p>
+ *          will be rewritten as:
+ *      </p>
+ *      <pre>{@code
+ *          "end_timestamp" > 1234 AND "begin_timestamp" < 5678
+ *      }</pre>
+ *
+ *      <p>
+ *          This ensures the filter applies to a superset of the actual result set, enabling safe 
+ *          pruning.
+ *      </p>
+ *   </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 +130,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'");
     }
 

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

@@ -58,6 +58,7 @@ public void setUp()
             List<ArchivesTableRow> values = new ArrayList<>();
 
             for (int j = 0; j < numValuesPerKey; j++) {
+
                 // We generate synthetic begin_timestamp and end_timestamp values for each split
                 // by offsetting two base timestamps (1700000000000L and 1705000000000L) with a
                 // fixed increment per split (10^10 * j).
@@ -82,26 +83,37 @@ public void tearDown()
     public void testListSplits()
     {
         for (Map.Entry<String, List<ArchivesTableRow>> entry : tableSplits.entrySet()) {
+
             // Without metadata filters
             compareListSplitsResult(entry, Optional.empty(), ImmutableList.of(0, 1, 2, 3, 4, 5, 6, 7, 8, 9));
+
             // query_begin_ts < archives_min_ts && query_end_ts > archives_max_ts
             compareListSplitsResult(entry, Optional.of("(end_timestamp > 1699999999999 AND begin_timestamp < 1795000000001)"), ImmutableList.of(0, 1, 2, 3, 4, 5, 6, 7, 8, 9));
+
             // query_begin_ts < archives_min_ts && query_end_ts > archives_min_ts && query_end_ts < archives_max_ts
             compareListSplitsResult(entry, Optional.of("(end_timestamp > 1699999999999 AND begin_timestamp < 1744999999999)"), ImmutableList.of(0, 1, 2, 3, 4));
+
             // query_end_ts < archives_min_ts
             compareListSplitsResult(entry, Optional.of("(begin_timestamp < 1699999999999)"), ImmutableList.of());
+
             // query_begin_ts > archives_min_ts && query_begin_ts < archives_max_ts && query_end_ts > archives_max_ts
             compareListSplitsResult(entry, Optional.of("(end_timestamp > 1745000000001 AND begin_timestamp < 1795000000001)"), ImmutableList.of(5, 6, 7, 8, 9));
+
             // query_begin_ts > archives_min_ts && query_begin_ts < archives_max_ts && query_end_ts < archives_max_ts
             compareListSplitsResult(entry, Optional.of("(end_timestamp > 1745000000001 AND begin_timestamp <= 1770000000000)"), ImmutableList.of(5, 6, 7));
+
             // query_begin_ts > archives_max_ts
             compareListSplitsResult(entry, Optional.of("(end_timestamp > 1795000000001)"), ImmutableList.of());
+
             // query_begin_ts = archive_min_ts
             compareListSplitsResult(entry, Optional.of("(end_timestamp >= 1700000000000 AND begin_timestamp <= 1700000000000)"), ImmutableList.of(0));
+
             // query_begin_ts = archive_max_ts
             compareListSplitsResult(entry, Optional.of("(end_timestamp >= 1795000000000 AND begin_timestamp <= 1795000000000)"), ImmutableList.of(9));
+
             // query_ts = x && x > archive_min_ts && x < archive_max_ts (non-exist)
             compareListSplitsResult(entry, Optional.of("(end_timestamp >= 1715000000001 AND begin_timestamp <= 1715000000001)"), ImmutableList.of());
+
             // query_ts = x && x > archive_min_ts && x < archive_max_ts
             compareListSplitsResult(entry, Optional.of("(end_timestamp >= 1715000000000 AND begin_timestamp <= 1715000000000)"), ImmutableList.of(1));
         }