resolve review comments

wraymo · wraymo · commit 1175af3e64b0 · 2025-09-05T10:00:45.000-04:00
diff --git a/presto-clp/src/main/java/com/facebook/presto/plugin/clp/optimization/ClpComputePushDown.java b/presto-clp/src/main/java/com/facebook/presto/plugin/clp/optimization/ClpComputePushDown.java
@@ -132,12 +132,12 @@ private PlanNode processFilter(FilterNode filterNode, TableScanNode tableScanNod
             boolean hasMetadataFilter = metadataSqlQuery.isPresent() && !metadataSqlQuery.get().isEmpty();
             if (hasMetadataFilter) {
                 metadataSqlQuery = Optional.of(splitFilterProvider.remapSplitFilterPushDownExpression(tableScope, metadataSqlQuery.get()));
-                log.debug("Metadata SQL query: %s", metadataSqlQuery);
+                log.debug("Metadata SQL query: %s", metadataSqlQuery.get());
             }
 
             if (kqlQuery.isPresent() || hasMetadataFilter) {
                 if (kqlQuery.isPresent()) {
-                    log.debug("KQL query: %s", kqlQuery);
+                    log.debug("KQL query: %s", kqlQuery.get());
                 }
 
                 ClpTableLayoutHandle layoutHandle = new ClpTableLayoutHandle(clpTableHandle, kqlQuery, metadataSqlQuery);
diff --git a/presto-clp/src/main/java/com/facebook/presto/plugin/clp/optimization/ClpUdfRewriter.java b/presto-clp/src/main/java/com/facebook/presto/plugin/clp/optimization/ClpUdfRewriter.java
@@ -49,7 +49,7 @@
 /**
  * Utility for rewriting CLP UDFs (e.g., <code>CLP_GET_*</code>) in {@link RowExpression} trees.
  * <p>
- * This optimizer traverses a query plan and rewrites calls to <code>CLP_GET_*</code> UDFs into
+ * Traverses a query plan and rewrites calls to <code>CLP_GET_*</code> UDFs into
  * {@link VariableReferenceExpression}s with meaningful names derived from their arguments.
  * <p>
  * This enables querying fields that are not part of the original table schema but are available
@@ -62,7 +62,7 @@ public final class ClpUdfRewriter
 
     public ClpUdfRewriter(FunctionMetadataManager functionManager)
     {
-        this.functionManager = requireNonNull(functionManager);
+        this.functionManager = requireNonNull(functionManager, "functionManager is null");
     }
 
     @Override
@@ -73,14 +73,14 @@ public PlanNode optimize(PlanNode maxSubplan, ConnectorSession session, Variable
 
     /**
      * Collects all existing variable assignments from {@link TableScanNode} instances that map
-     * column handles to {@link VariableReferenceExpression}s.
+     * {@link ColumnHandle}s to {@link VariableReferenceExpression}s.
      * <p>
      * This method traverses the given plan subtree, visiting the {@link TableScanNode} and
      * extracting its assignments. The resulting map allows tracking which scan-level variables
      * already exist for specific columns, so that subsequent optimizer passes (e.g., CLP UDF
      * rewriting) can reuse them instead of creating duplicates.
      * <p>
-     * The returned map is keyed by the column handle, and the value is the
+     * The key of the returned map is the {@link ColumnHandle}, and the value is the
      * {@link VariableReferenceExpression} assigned to it in the scan node.
      *
      * @param root the root {@link PlanNode} of the plan subtree
@@ -137,24 +137,26 @@ public PlanNode visitFilter(FilterNode node, RewriteContext<Void> context)
         }
 
         /**
-         * Rewrites <code>CLP_GET_*</code> UDFs in a {@link RowExpression}, collecting each resulting
-         * variable into the given map along with its associated {@link ColumnHandle}.
+         * Rewrites <code>CLP_GET_*</code> UDFs in a {@link RowExpression}, collecting each
+         * resulting variable into the given map along with its associated {@link ColumnHandle}.
          * <p>
-         * Each <code>CLP_GET_*</code> UDF must take a single constant string argument, which is used to
-         * construct the name of the variable reference (e.g. <code>CLP_GET_STRING('foo')</code> becomes
-         * a variable name <code>foo</code>). Invalid usages (e.g., non-constant arguments) will throw a
-         * {@link PrestoException}.
+         * Each <code>CLP_GET_*</code> UDF must take a single constant string argument, which is
+         * used to construct the name of the variable reference (e.g.
+         * <code>CLP_GET_STRING('foo')</code> becomes a variable name <code>foo</code>). Invalid
+         * usages (e.g., non-constant arguments) will throw a {@link PrestoException}.
          *
          * @param expression the input expression to analyze and possibly rewrite
          * @param functionManager function manager used to resolve function metadata
          * @param variableAllocator variable allocator used to create new variable references
-         * @return a possibly rewritten {@link RowExpression} with <code>CLP_GET_*</code> calls replaced
+         * @return a possibly rewritten {@link RowExpression} with <code>CLP_GET_*</code> calls
+         * replaced
          */
         private RowExpression rewriteClpUdfs(
                 RowExpression expression,
                 FunctionMetadataManager functionManager,
                 VariableAllocator variableAllocator)
         {
+            // Handle CLP_GET_* function calls
             if (expression instanceof CallExpression) {
                 CallExpression call = (CallExpression) expression;
                 String functionName = functionManager.getFunctionMetadata(call.getFunctionHandle()).getName().getObjectName().toUpperCase();
@@ -191,6 +193,7 @@ private RowExpression rewriteClpUdfs(
                 return new CallExpression(call.getDisplayName(), call.getFunctionHandle(), call.getType(), rewrittenArgs);
             }
 
+            // Handle special forms (e.g., AND, OR, etc.)
             if (expression instanceof SpecialFormExpression) {
                 SpecialFormExpression special = (SpecialFormExpression) expression;
 
@@ -258,8 +261,8 @@ else if (c == '_') {
         }
 
         /**
-         * Builds a new {@link TableScanNode} that includes additional variables and column handles
-         * for rewritten CLP UDFs.
+         * Builds a new {@link TableScanNode} that includes additional
+         * {@link VariableReferenceExpression}s and {@link ColumnHandle}s for rewritten CLP UDFs.
          *
          * @param node the original table scan node
          * @return the updated table scan node
diff --git a/presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpUdfRewriter.java b/presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpUdfRewriter.java
@@ -15,6 +15,7 @@
 
 import com.facebook.presto.Session;
 import com.facebook.presto.common.transaction.TransactionId;
+import com.facebook.presto.common.type.ArrayType;
 import com.facebook.presto.cost.PlanNodeStatsEstimate;
 import com.facebook.presto.cost.StatsAndCosts;
 import com.facebook.presto.cost.StatsProvider;
@@ -56,6 +57,7 @@
 
 import static com.facebook.presto.common.Utils.checkState;
 import static com.facebook.presto.common.type.BigintType.BIGINT;
+import static com.facebook.presto.common.type.DoubleType.DOUBLE;
 import static com.facebook.presto.common.type.VarcharType.VARCHAR;
 import static com.facebook.presto.metadata.FunctionExtractor.extractFunctions;
 import static com.facebook.presto.plugin.clp.ClpMetadataDbSetUp.ARCHIVES_STORAGE_DIRECTORY_BASE;
@@ -145,7 +147,8 @@ public void testScanFilter()
 
         Plan plan = localQueryRunner.createPlan(
                 session,
-                "SELECT * FROM test WHERE CLP_GET_INT('user_id') = 0 AND LOWER(city.Name) = 'BEIJING'",
+                "SELECT * FROM test WHERE CLP_GET_INT('user_id') = 0 AND CLP_GET_FLOAT('fare') < 50.0 AND CLP_GET_STRING('city') = 'SF' AND " +
+                        "CLP_GET_BOOL('isHoliday') = true AND cardinality(CLP_GET_STRING_ARRAY('tags')) > 0 AND LOWER(city.Name) = 'BEIJING'",
                 WarningCollector.NOOP);
         ClpUdfRewriter udfRewriter = new ClpUdfRewriter(functionAndTypeManager);
         PlanNode optimizedPlan = udfRewriter.optimize(plan.getRoot(), session.toConnectorSession(), variableAllocator, planNodeIdAllocator);
@@ -159,10 +162,20 @@ public void testScanFilter()
                 new Plan(optimizedPlan, plan.getTypes(), StatsAndCosts.empty()),
                 anyTree(
                         filter(
-                                expression("lower(city.Name) = 'BEIJING'"),
+                                expression("lower(city.Name) = 'BEIJING' AND cardinality(tags) > 0"),
                                 ClpTableScanMatcher.clpTableScanPattern(
-                                        new ClpTableLayoutHandle(table, Optional.of("(user_id: 0)"), Optional.empty()),
-                                        ImmutableSet.of(city, fare, isHoliday, new ClpColumnHandle("user_id", BIGINT))))));
+                                        new ClpTableLayoutHandle(
+                                                table,
+                                                Optional.of(
+                                                        "(((user_id: 0 AND fare < 50.0) AND (city: \"SF\" AND isHoliday: true)))"),
+                                                Optional.empty()),
+                                        ImmutableSet.of(
+                                                city,
+                                                fare,
+                                                isHoliday,
+                                                new ClpColumnHandle("user_id", BIGINT),
+                                                new ClpColumnHandle("city", VARCHAR),
+                                                new ClpColumnHandle("tags", new ArrayType(VARCHAR)))))));
     }
 
     @Test
@@ -173,7 +186,8 @@ public void testScanProject()
 
         Plan plan = localQueryRunner.createPlan(
                 session,
-                "SELECT CLP_GET_STRING('user'), city.Name FROM test",
+                "SELECT CLP_GET_INT('user_id'), CLP_GET_FLOAT('fare'), CLP_GET_STRING('user'), " +
+                        "CLP_GET_BOOL('isHoliday'), CLP_GET_STRING_ARRAY('tags'), city.Name from test",
                 WarningCollector.NOOP);
         ClpUdfRewriter udfRewriter = new ClpUdfRewriter(functionAndTypeManager);
         PlanNode optimizedPlan = udfRewriter.optimize(plan.getRoot(), session.toConnectorSession(), variableAllocator, planNodeIdAllocator);
@@ -188,8 +202,16 @@ public void testScanProject()
                 anyTree(
                         project(
                                 ImmutableMap.of(
+                                        "clp_get_int",
+                                        PlanMatchPattern.expression("user_id"),
+                                        "clp_get_float",
+                                        PlanMatchPattern.expression("fare"),
                                         "clp_get_string",
                                         PlanMatchPattern.expression("user"),
+                                        "clp_get_bool",
+                                        PlanMatchPattern.expression("isHoliday"),
+                                        "clp_get_string_array",
+                                        PlanMatchPattern.expression("tags"),
                                         "expr",
                                         PlanMatchPattern.expression("city.Name")),
                                 ClpTableScanMatcher.clpTableScanPattern(
@@ -198,7 +220,11 @@ public void testScanProject()
                                                 Optional.empty(),
                                                 Optional.empty()),
                                         ImmutableSet.of(
+                                                new ClpColumnHandle("user_id", BIGINT),
+                                                new ClpColumnHandle("fare", DOUBLE),
                                                 new ClpColumnHandle("user", VARCHAR),
+                                                isHoliday,
+                                                new ClpColumnHandle("tags", new ArrayType(VARCHAR)),
                                                 city)))));
     }
 

Original file line number	Diff line number	Diff line change
`@@ -132,12 +132,12 @@ private PlanNode processFilter(FilterNode filterNode, TableScanNode tableScanNod`
`132`	`132`	`boolean hasMetadataFilter = metadataSqlQuery.isPresent() && !metadataSqlQuery.get().isEmpty();`
`133`	`133`	`if (hasMetadataFilter) {`
`134`	`134`	`metadataSqlQuery = Optional.of(splitFilterProvider.remapSplitFilterPushDownExpression(tableScope, metadataSqlQuery.get()));`
`135`		`- log.debug("Metadata SQL query: %s", metadataSqlQuery);`
	`135`	`+ log.debug("Metadata SQL query: %s", metadataSqlQuery.get());`
`136`	`136`	`}`
`137`	`137`
`138`	`138`	`if (kqlQuery.isPresent() \|\| hasMetadataFilter) {`
`139`	`139`	`if (kqlQuery.isPresent()) {`
`140`		`- log.debug("KQL query: %s", kqlQuery);`
	`140`	`+ log.debug("KQL query: %s", kqlQuery.get());`
`141`	`141`	`}`
`142`	`142`
`143`	`143`	`ClpTableLayoutHandle layoutHandle = new ClpTableLayoutHandle(clpTableHandle, kqlQuery, metadataSqlQuery);`