From 3faed0b10ce9a645d16d5070485d883aea2e046a Mon Sep 17 00:00:00 2001 From: wraymo Date: Mon, 14 Jul 2025 09:08:28 -0400 Subject: [PATCH 1/4] add context --- .../plugin/clp/ClpFilterToKqlConverter.java | 159 ++++++++++-------- .../presto/plugin/clp/ClpPlanOptimizer.java | 6 +- .../presto/plugin/clp/TestClpFilterToKql.java | 21 ++- 3 files changed, 108 insertions(+), 78 deletions(-) diff --git a/presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpFilterToKqlConverter.java b/presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpFilterToKqlConverter.java index 7e6fae13fdb8f..632f5f753f9dc 100644 --- a/presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpFilterToKqlConverter.java +++ b/presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpFilterToKqlConverter.java @@ -95,7 +95,7 @@ * */ public class ClpFilterToKqlConverter - implements RowExpressionVisitor + implements RowExpressionVisitor> { private static final Set LOGICAL_BINARY_OPS_FILTER = ImmutableSet.of(EQUAL, NOT_EQUAL, LESS_THAN, LESS_THAN_OR_EQUAL, GREATER_THAN, GREATER_THAN_OR_EQUAL); @@ -118,15 +118,15 @@ public ClpFilterToKqlConverter( } @Override - public ClpExpression visitCall(CallExpression node, Void context) + public ClpExpression visitCall(CallExpression node, Set context) { FunctionHandle functionHandle = node.getFunctionHandle(); if (standardFunctionResolution.isNotFunction(functionHandle)) { - return handleNot(node); + return handleNot(node, context); } if (standardFunctionResolution.isLikeFunction(functionHandle)) { - return handleLike(node); + return handleLike(node, context); } FunctionMetadata functionMetadata = functionMetadataManager.getFunctionMetadata(node.getFunctionHandle()); @@ -134,10 +134,10 @@ public ClpExpression visitCall(CallExpression node, Void context) if (operatorTypeOptional.isPresent()) { OperatorType operatorType = operatorTypeOptional.get(); if (operatorType.isComparisonOperator() && operatorType != IS_DISTINCT_FROM) { - return handleLogicalBinary(operatorType, node); + return handleLogicalBinary(operatorType, node, context); } if (BETWEEN == operatorType) { - return handleBetween(node); + return handleBetween(node, context); } } @@ -145,38 +145,38 @@ public ClpExpression visitCall(CallExpression node, Void context) } @Override - public ClpExpression visitConstant(ConstantExpression node, Void context) + public ClpExpression visitConstant(ConstantExpression node, Set context) { return new ClpExpression(getLiteralString(node)); } @Override - public ClpExpression visitVariableReference(VariableReferenceExpression node, Void context) + public ClpExpression visitVariableReference(VariableReferenceExpression node, Set context) { return new ClpExpression(getVariableName(node)); } @Override - public ClpExpression visitSpecialForm(SpecialFormExpression node, Void context) + public ClpExpression visitSpecialForm(SpecialFormExpression node, Set context) { switch (node.getForm()) { case AND: - return handleAnd(node); + return handleAnd(node, context); case OR: - return handleOr(node); + return handleOr(node, context); case IN: - return handleIn(node); + return handleIn(node, context); case IS_NULL: - return handleIsNull(node); + return handleIsNull(node, context); case DEREFERENCE: - return handleDereference(node); + return handleDereference(node, context); default: return new ClpExpression(node); } } @Override - public ClpExpression visitExpression(RowExpression node, Void context) + public ClpExpression visitExpression(RowExpression node, Set context) { // For all other expressions, return the original expression return new ClpExpression(node); @@ -219,11 +219,12 @@ private String getVariableName(VariableReferenceExpression variable) *

* Example: col1 BETWEEN 0 AND 5 → col1 >= 0 AND col1 <= 5 * - * @param node the {@code BETWEEN} call expression + * @param node the BETWEEN call expression + * @param context a set of VariableReferenceExpressions used for pushdown; * @return a ClpExpression containing either the equivalent KQL query, or the original * expression if it couldn't be translated */ - private ClpExpression handleBetween(CallExpression node) + private ClpExpression handleBetween(CallExpression node, Set context) { List arguments = node.getArguments(); if (arguments.size() != 3) { @@ -243,7 +244,7 @@ private ClpExpression handleBetween(CallExpression node) || !isClpCompatibleNumericType(third.getType())) { return new ClpExpression(node); } - Optional variableOpt = first.accept(this, null).getPushDownExpression(); + Optional variableOpt = first.accept(this, context).getPushDownExpression(); if (!variableOpt.isPresent()) { return new ClpExpression(node); } @@ -263,10 +264,11 @@ private ClpExpression handleBetween(CallExpression node) * Example: NOT (col1 = 5) → NOT col1: 5 * * @param node the NOT call expression + * @param context a set of VariableReferenceExpressions used for pushdown; * @return a ClpExpression containing either the equivalent KQL query, or the original * expression if it couldn't be translated */ - private ClpExpression handleNot(CallExpression node) + private ClpExpression handleNot(CallExpression node, Set context) { if (node.getArguments().size() != 1) { throw new PrestoException(CLP_PUSHDOWN_UNSUPPORTED_EXPRESSION, @@ -274,7 +276,7 @@ private ClpExpression handleNot(CallExpression node) } RowExpression input = node.getArguments().get(0); - ClpExpression expression = input.accept(this, null); + ClpExpression expression = input.accept(this, context); if (expression.getRemainingExpression().isPresent() || !expression.getPushDownExpression().isPresent()) { return new ClpExpression(node); } @@ -297,15 +299,16 @@ private ClpExpression handleNot(CallExpression node) * Example: col1 LIKE 'a_bc%' → col1: "a?bc*" * * @param node the LIKE call expression + * @param context a set of VariableReferenceExpressions used for pushdown; * @return a ClpExpression containing either the equivalent KQL query, or the original * expression if it couldn't be translated */ - private ClpExpression handleLike(CallExpression node) + private ClpExpression handleLike(CallExpression node, Set context) { if (node.getArguments().size() != 2) { throw new PrestoException(CLP_PUSHDOWN_UNSUPPORTED_EXPRESSION, "LIKE operator must have exactly two arguments. Received: " + node); } - ClpExpression variable = node.getArguments().get(0).accept(this, null); + ClpExpression variable = node.getArguments().get(0).accept(this, context); if (!variable.getPushDownExpression().isPresent()) { return new ClpExpression(node); } @@ -346,10 +349,11 @@ else if (argument instanceof CallExpression) { * * @param operator the binary operator (e.g., EQUAL, NOT_EQUAL) * @param node the call expression representing the binary operation + * @param context a set of VariableReferenceExpressions used for pushdown; * @return a ClpExpression containing either the equivalent KQL query, or the original * expression if it couldn't be translated */ - private ClpExpression handleLogicalBinary(OperatorType operator, CallExpression node) + private ClpExpression handleLogicalBinary(OperatorType operator, CallExpression node, Set context) { if (node.getArguments().size() != 2) { throw new PrestoException(CLP_PUSHDOWN_UNSUPPORTED_EXPRESSION, @@ -358,21 +362,21 @@ private ClpExpression handleLogicalBinary(OperatorType operator, CallExpression RowExpression left = node.getArguments().get(0); RowExpression right = node.getArguments().get(1); - ClpExpression maybeLeftSubstring = tryInterpretSubstringEquality(operator, left, right); - if (maybeLeftSubstring.getPushDownExpression().isPresent()) { - return maybeLeftSubstring; + Optional maybeLeftSubstring = tryInterpretSubstringEquality(operator, left, right, context); + if (maybeLeftSubstring.isPresent()) { + return maybeLeftSubstring.get(); } - ClpExpression maybeRightSubstring = tryInterpretSubstringEquality(operator, right, left); - if (maybeRightSubstring.getPushDownExpression().isPresent()) { - return maybeRightSubstring; + Optional maybeRightSubstring = tryInterpretSubstringEquality(operator, right, left, context); + if (maybeRightSubstring.isPresent()) { + return maybeRightSubstring.get(); } - ClpExpression leftExpression = left.accept(this, null); - ClpExpression rightExpression = right.accept(this, null); - Optional leftDefinition = leftExpression.getPushDownExpression(); - Optional rightDefinition = rightExpression.getPushDownExpression(); - if (!leftDefinition.isPresent() || !rightDefinition.isPresent()) { + ClpExpression leftExpression = left.accept(this, context); + ClpExpression rightExpression = right.accept(this, context); + Optional leftPushDownExpression = leftExpression.getPushDownExpression(); + Optional rightPushDownExpression = rightExpression.getPushDownExpression(); + if (!leftPushDownExpression.isPresent() || !rightPushDownExpression.isPresent()) { return new ClpExpression(node); } @@ -384,8 +388,8 @@ private ClpExpression handleLogicalBinary(OperatorType operator, CallExpression if (rightIsConstant) { return buildClpExpression( - leftDefinition.get(), // variable - rightDefinition.get(), // literal + leftPushDownExpression.get(), // variable + rightPushDownExpression.get(), // literal operator, rightType, node); @@ -393,8 +397,8 @@ private ClpExpression handleLogicalBinary(OperatorType operator, CallExpression else if (leftIsConstant) { OperatorType newOperator = flip(operator); return buildClpExpression( - rightDefinition.get(), // variable - leftDefinition.get(), // literal + rightPushDownExpression.get(), // variable + leftPushDownExpression.get(), // literal newOperator, leftType, node); @@ -471,26 +475,27 @@ else if (LOGICAL_BINARY_OPS_FILTER.contains(operator) && !(literalType instanceo * @param operator the comparison operator (should be EQUAL) * @param possibleSubstring the left or right expression, possibly a SUBSTR call * @param possibleLiteral the opposite expression, possibly a string constant - * @return a ClpExpression containing either the equivalent KQL query, or nothing if it couldn't - * be translated + * @param context a set of VariableReferenceExpressions used for pushdown; + * @return an Optional containing a ClpExpression with the equivalent KQL query */ - private ClpExpression tryInterpretSubstringEquality( + private Optional tryInterpretSubstringEquality( OperatorType operator, RowExpression possibleSubstring, - RowExpression possibleLiteral) + RowExpression possibleLiteral, + Set context) { if (!operator.equals(EQUAL)) { - return new ClpExpression(); + return Optional.empty(); } if (!(possibleSubstring instanceof CallExpression) || !(possibleLiteral instanceof ConstantExpression)) { - return new ClpExpression(); + return Optional.empty(); } - Optional maybeSubstringCall = parseSubstringCall((CallExpression) possibleSubstring); + Optional maybeSubstringCall = parseSubstringCall((CallExpression) possibleSubstring, context); if (!maybeSubstringCall.isPresent()) { - return new ClpExpression(); + return Optional.empty(); } String targetString = getLiteralString((ConstantExpression) possibleLiteral); @@ -501,9 +506,10 @@ private ClpExpression tryInterpretSubstringEquality( * Parses a SUBSTR(x, start [, length]) call into a SubstrInfo object if valid. * * @param callExpression the call expression to inspect + * @param context a set of VariableReferenceExpressions used for pushdown; * @return an Optional containing SubstrInfo if the expression is a valid SUBSTR call */ - private Optional parseSubstringCall(CallExpression callExpression) + private Optional parseSubstringCall(CallExpression callExpression, Set context) { FunctionMetadata functionMetadata = functionMetadataManager.getFunctionMetadata(callExpression.getFunctionHandle()); String functionName = functionMetadata.getName().getObjectName(); @@ -516,7 +522,7 @@ private Optional parseSubstringCall(CallExpression callExpression) return Optional.empty(); } - ClpExpression variable = callExpression.getArguments().get(0).accept(this, null); + ClpExpression variable = callExpression.getArguments().get(0).accept(this, context); if (!variable.getPushDownExpression().isPresent()) { return Optional.empty(); } @@ -545,10 +551,9 @@ private Optional parseSubstringCall(CallExpression callExpression) * * @param info parsed SUBSTR call info * @param targetString the literal string being compared to - * @return a ClpExpression containing either the equivalent KQL query, or nothing if it couldn't - * be translated + * @return an Optional containing either a ClpExpression with the equivalent KQL query */ - private ClpExpression interpretSubstringEquality(SubstrInfo info, String targetString) + private Optional interpretSubstringEquality(SubstrInfo info, String targetString) { if (info.lengthExpression != null) { Optional maybeStart = parseIntValue(info.startExpression); @@ -564,7 +569,7 @@ private ClpExpression interpretSubstringEquality(SubstrInfo info, String targetS result.append("?"); } result.append(targetString).append("*\""); - return new ClpExpression(result.toString()); + return Optional.of(new ClpExpression(result.toString())); } } } @@ -579,15 +584,15 @@ private ClpExpression interpretSubstringEquality(SubstrInfo info, String targetS result.append("?"); } result.append(targetString).append("\""); - return new ClpExpression(result.toString()); + return Optional.of(new ClpExpression(result.toString())); } if (start == -targetString.length()) { - return new ClpExpression(format("%s: \"*%s\"", info.variableName, targetString)); + return Optional.of(new ClpExpression(format("%s: \"*%s\"", info.variableName, targetString))); } } } - return new ClpExpression(); + return Optional.empty(); } /** @@ -655,9 +660,10 @@ private Optional parseLengthLiteral(RowExpression lengthExpression, Str * Example: col1 = 5 AND col2 = 'abc' → (col1: 5 AND col2: "abc") * * @param node the AND special form expression + * @param context a set of VariableReferenceExpressions used for pushdown; * @return a ClpExpression containing the KQL query and any remaining sub-expressions */ - private ClpExpression handleAnd(SpecialFormExpression node) + private ClpExpression handleAnd(SpecialFormExpression node, Set context) { StringBuilder metadataQueryBuilder = new StringBuilder(); metadataQueryBuilder.append("("); @@ -667,7 +673,7 @@ private ClpExpression handleAnd(SpecialFormExpression node) boolean hasMetadataSql = false; boolean hasPushDownExpression = false; for (RowExpression argument : node.getArguments()) { - ClpExpression expression = argument.accept(this, null); + ClpExpression expression = argument.accept(this, context); if (expression.getPushDownExpression().isPresent()) { hasPushDownExpression = true; queryBuilder.append(expression.getPushDownExpression().get()); @@ -713,20 +719,25 @@ else if (!remainingExpressions.isEmpty()) { * Example: col1 = 5 OR col1 = 10 → (col1: 5 OR col1: 10) * * @param node the OR special form expression + * @param context a set of VariableReferenceExpressions used for pushdown; * @return a ClpExpression containing either the equivalent KQL query, or the original * expression if it couldn't be fully translated */ - private ClpExpression handleOr(SpecialFormExpression node) + private ClpExpression handleOr(SpecialFormExpression node, Set context) { StringBuilder metadataQueryBuilder = new StringBuilder(); metadataQueryBuilder.append("("); StringBuilder queryBuilder = new StringBuilder(); queryBuilder.append("("); + boolean allPushedDown = true; boolean hasAllMetadataSql = true; for (RowExpression argument : node.getArguments()) { - ClpExpression expression = argument.accept(this, null); + ClpExpression expression = argument.accept(this, context); + // Note: It is possible in the future that an expression cannot be pushed down as a KQL query, but can be + // pushed down as a metadata SQL query. if (expression.getRemainingExpression().isPresent() || !expression.getPushDownExpression().isPresent()) { - return new ClpExpression(node); + allPushedDown = false; + continue; } queryBuilder.append(expression.getPushDownExpression().get()); queryBuilder.append(" OR "); @@ -738,10 +749,13 @@ private ClpExpression handleOr(SpecialFormExpression node) hasAllMetadataSql = false; } } - // Remove the last " OR " from the query - return new ClpExpression( - queryBuilder.substring(0, queryBuilder.length() - 4) + ")", - hasAllMetadataSql ? metadataQueryBuilder.substring(0, metadataQueryBuilder.length() - 4) + ")" : null); + if (allPushedDown) { + // Remove the last " OR " from the query + return new ClpExpression( + queryBuilder.substring(0, queryBuilder.length() - 4) + ")", + hasAllMetadataSql ? metadataQueryBuilder.substring(0, metadataQueryBuilder.length() - 4) + ")" : null); + } + return new ClpExpression(node); } /** @@ -750,12 +764,13 @@ private ClpExpression handleOr(SpecialFormExpression node) * Example: col1 IN (1, 2, 3) → (col1: 1 OR col1: 2 OR col1: 3) * * @param node the IN special form expression + * @param context a set of VariableReferenceExpressions used for pushdown; * @return a ClpExpression containing either the equivalent KQL query, or the original * expression if it couldn't be translated */ - private ClpExpression handleIn(SpecialFormExpression node) + private ClpExpression handleIn(SpecialFormExpression node, Set context) { - ClpExpression variable = node.getArguments().get(0).accept(this, null); + ClpExpression variable = node.getArguments().get(0).accept(this, context); if (!variable.getPushDownExpression().isPresent()) { return new ClpExpression(node); } @@ -788,17 +803,18 @@ private ClpExpression handleIn(SpecialFormExpression node) * Example: col1 IS NULL → NOT col1: * * * @param node the IS_NULL special form expression + * @param context a set of VariableReferenceExpressions used for pushdown; * @return a ClpExpression containing either the equivalent KQL query, or the original * expression if it couldn't be translated */ - private ClpExpression handleIsNull(SpecialFormExpression node) + private ClpExpression handleIsNull(SpecialFormExpression node, Set context) { if (node.getArguments().size() != 1) { throw new PrestoException(CLP_PUSHDOWN_UNSUPPORTED_EXPRESSION, "IS NULL operator must have exactly one argument. Received: " + node); } - ClpExpression expression = node.getArguments().get(0).accept(this, null); + ClpExpression expression = node.getArguments().get(0).accept(this, context); if (!expression.getPushDownExpression().isPresent()) { return new ClpExpression(node); } @@ -816,13 +832,14 @@ private ClpExpression handleIsNull(SpecialFormExpression node) * * @param expression the dereference expression ({@link SpecialFormExpression} or * {@link VariableReferenceExpression}) + * @param context a set of VariableReferenceExpressions used for pushdown; * @return a ClpExpression containing either the dot-separated field name, or the original * expression if it couldn't be translated */ - private ClpExpression handleDereference(RowExpression expression) + private ClpExpression handleDereference(RowExpression expression, Set context) { if (expression instanceof VariableReferenceExpression) { - return expression.accept(this, null); + return expression.accept(this, context); } if (!(expression instanceof SpecialFormExpression)) { @@ -862,7 +879,7 @@ private ClpExpression handleDereference(RowExpression expression) RowType.Field field = rowType.getFields().get(fieldIndex); String fieldName = field.getName().orElse("field" + fieldIndex); - ClpExpression baseString = handleDereference(base); + ClpExpression baseString = handleDereference(base, context); if (!baseString.getPushDownExpression().isPresent()) { return new ClpExpression(expression); } 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 index 157af459cb80e..0e67e5b76c467 100644 --- 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 @@ -29,8 +29,10 @@ import com.facebook.presto.spi.relation.RowExpression; import com.facebook.presto.spi.relation.VariableReferenceExpression; +import java.util.HashSet; import java.util.Map; import java.util.Optional; +import java.util.Set; import static com.facebook.presto.plugin.clp.ClpConnectorFactory.CONNECTOR_NAME; import static com.facebook.presto.spi.ConnectorPlanRewriter.rewriteWith; @@ -79,12 +81,14 @@ public PlanNode visitFilter(FilterNode node, RewriteContext context) TableHandle tableHandle = tableScanNode.getTable(); ClpTableHandle clpTableHandle = (ClpTableHandle) tableHandle.getConnectorHandle(); String scope = CONNECTOR_NAME + "." + clpTableHandle.getSchemaTableName().toString(); + Set clpUdfVariablesInFilterNode = new HashSet<>(); ClpExpression clpExpression = node.getPredicate().accept( new ClpFilterToKqlConverter( functionResolution, functionManager, assignments, - metadataFilterProvider.getColumnNames(scope)), null); + metadataFilterProvider.getColumnNames(scope)), + clpUdfVariablesInFilterNode); Optional kqlQuery = clpExpression.getPushDownExpression(); Optional metadataSqlQuery = clpExpression.getMetadataSqlQuery(); Optional remainingPredicate = clpExpression.getRemainingExpression(); diff --git a/presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpFilterToKql.java b/presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpFilterToKql.java index 5007a0ff10bd3..6fe74105da970 100644 --- a/presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpFilterToKql.java +++ b/presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpFilterToKql.java @@ -14,9 +14,11 @@ package com.facebook.presto.plugin.clp; import com.facebook.presto.spi.relation.RowExpression; +import com.facebook.presto.spi.relation.VariableReferenceExpression; import com.google.common.collect.ImmutableSet; import org.testng.annotations.Test; +import java.util.HashSet; import java.util.Optional; import java.util.Set; @@ -268,14 +270,16 @@ public void testMetadataSqlGeneration() private void testPushDown(SessionHolder sessionHolder, String sql, String expectedKql, String expectedRemaining) { - ClpExpression clpExpression = tryPushDown(sql, sessionHolder, ImmutableSet.of()); - testFilter(clpExpression, expectedKql, expectedRemaining, sessionHolder); + HashSet clpUdfVariables = new HashSet<>(); + ClpExpression clpExpression = tryPushDown(sql, sessionHolder, ImmutableSet.of(), clpUdfVariables); + testFilter(clpExpression, expectedKql, expectedRemaining, clpUdfVariables, sessionHolder); } private void testPushDown(SessionHolder sessionHolder, String sql, String expectedKql, String expectedMetadataSqlQuery, Set metadataFilterColumns) { - ClpExpression clpExpression = tryPushDown(sql, sessionHolder, metadataFilterColumns); - testFilter(clpExpression, expectedKql, null, sessionHolder); + HashSet clpUdfVariables = new HashSet<>(); + ClpExpression clpExpression = tryPushDown(sql, sessionHolder, metadataFilterColumns, clpUdfVariables); + testFilter(clpExpression, expectedKql, null, clpUdfVariables, sessionHolder); if (expectedMetadataSqlQuery != null) { assertTrue(clpExpression.getMetadataSqlQuery().isPresent()); assertEquals(clpExpression.getMetadataSqlQuery().get(), expectedMetadataSqlQuery); @@ -285,7 +289,11 @@ private void testPushDown(SessionHolder sessionHolder, String sql, String expect } } - private ClpExpression tryPushDown(String sqlExpression, SessionHolder sessionHolder, Set metadataFilterColumns) + private ClpExpression tryPushDown( + String sqlExpression, + SessionHolder sessionHolder, + Set metadataFilterColumns, + Set clpUdfVariables) { RowExpression pushDownExpression = getRowExpression(sqlExpression, sessionHolder); return pushDownExpression.accept( @@ -294,13 +302,14 @@ private ClpExpression tryPushDown(String sqlExpression, SessionHolder sessionHol functionAndTypeManager, variableToColumnHandleMap, metadataFilterColumns), - null); + clpUdfVariables); } private void testFilter( ClpExpression clpExpression, String expectedKqlExpression, String expectedRemainingExpression, + Set clpUdfVariables, SessionHolder sessionHolder) { Optional kqlExpression = clpExpression.getPushDownExpression(); From 346476f6d155b3480bec057bb7bbde9136c8a72b Mon Sep 17 00:00:00 2001 From: wraymo Date: Mon, 14 Jul 2025 10:02:44 -0400 Subject: [PATCH 2/4] remove assignments in ClpFilterToKqlConverter --- .../plugin/clp/ClpFilterToKqlConverter.java | 66 +++++++++---------- .../presto/plugin/clp/ClpPlanOptimizer.java | 9 +-- .../presto/plugin/clp/TestClpFilterToKql.java | 7 +- 3 files changed, 40 insertions(+), 42 deletions(-) diff --git a/presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpFilterToKqlConverter.java b/presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpFilterToKqlConverter.java index 632f5f753f9dc..4fdecf943de90 100644 --- a/presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpFilterToKqlConverter.java +++ b/presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpFilterToKqlConverter.java @@ -95,30 +95,27 @@ * */ public class ClpFilterToKqlConverter - implements RowExpressionVisitor> + implements RowExpressionVisitor> { private static final Set LOGICAL_BINARY_OPS_FILTER = ImmutableSet.of(EQUAL, NOT_EQUAL, LESS_THAN, LESS_THAN_OR_EQUAL, GREATER_THAN, GREATER_THAN_OR_EQUAL); private final StandardFunctionResolution standardFunctionResolution; private final FunctionMetadataManager functionMetadataManager; - private final Map assignments; private final Set metadataFilterColumns; public ClpFilterToKqlConverter( StandardFunctionResolution standardFunctionResolution, FunctionMetadataManager functionMetadataManager, - Map assignments, Set metadataFilterColumns) { this.standardFunctionResolution = requireNonNull(standardFunctionResolution, "standardFunctionResolution is null"); this.functionMetadataManager = requireNonNull(functionMetadataManager, "function metadata manager is null"); - this.assignments = requireNonNull(assignments, "assignments is null"); this.metadataFilterColumns = requireNonNull(metadataFilterColumns, "metadataFilterColumns is null"); } @Override - public ClpExpression visitCall(CallExpression node, Set context) + public ClpExpression visitCall(CallExpression node, Map context) { FunctionHandle functionHandle = node.getFunctionHandle(); if (standardFunctionResolution.isNotFunction(functionHandle)) { @@ -145,19 +142,19 @@ public ClpExpression visitCall(CallExpression node, Set context) + public ClpExpression visitConstant(ConstantExpression node, Map context) { return new ClpExpression(getLiteralString(node)); } @Override - public ClpExpression visitVariableReference(VariableReferenceExpression node, Set context) + public ClpExpression visitVariableReference(VariableReferenceExpression node, Map context) { - return new ClpExpression(getVariableName(node)); + return new ClpExpression(getVariableName(node, context)); } @Override - public ClpExpression visitSpecialForm(SpecialFormExpression node, Set context) + public ClpExpression visitSpecialForm(SpecialFormExpression node, Map context) { switch (node.getForm()) { case AND: @@ -176,7 +173,7 @@ public ClpExpression visitSpecialForm(SpecialFormExpression node, Set context) + public ClpExpression visitExpression(RowExpression node, Map context) { // For all other expressions, return the original expression return new ClpExpression(node); @@ -200,11 +197,12 @@ private String getLiteralString(ConstantExpression literal) * Retrieves the original column name from a variable reference. * * @param variable the variable reference expression + * @param context a mapping from variable references to column handles used for pushdown * @return the original column name as a string */ - private String getVariableName(VariableReferenceExpression variable) + private String getVariableName(VariableReferenceExpression variable, Map context) { - return ((ClpColumnHandle) assignments.get(variable)).getOriginalColumnName(); + return ((ClpColumnHandle) context.get(variable)).getOriginalColumnName(); } /** @@ -220,11 +218,11 @@ private String getVariableName(VariableReferenceExpression variable) * Example: col1 BETWEEN 0 AND 5 → col1 >= 0 AND col1 <= 5 * * @param node the BETWEEN call expression - * @param context a set of VariableReferenceExpressions used for pushdown; + * @param context a mapping from variable references to column handles used for pushdown * @return a ClpExpression containing either the equivalent KQL query, or the original * expression if it couldn't be translated */ - private ClpExpression handleBetween(CallExpression node, Set context) + private ClpExpression handleBetween(CallExpression node, Map context) { List arguments = node.getArguments(); if (arguments.size() != 3) { @@ -264,11 +262,11 @@ private ClpExpression handleBetween(CallExpression node, SetNOT (col1 = 5) → NOT col1: 5 * * @param node the NOT call expression - * @param context a set of VariableReferenceExpressions used for pushdown; + * @param context a mapping from variable references to column handles used for pushdown * @return a ClpExpression containing either the equivalent KQL query, or the original * expression if it couldn't be translated */ - private ClpExpression handleNot(CallExpression node, Set context) + private ClpExpression handleNot(CallExpression node, Map context) { if (node.getArguments().size() != 1) { throw new PrestoException(CLP_PUSHDOWN_UNSUPPORTED_EXPRESSION, @@ -299,11 +297,11 @@ private ClpExpression handleNot(CallExpression node, Setcol1 LIKE 'a_bc%' → col1: "a?bc*" * * @param node the LIKE call expression - * @param context a set of VariableReferenceExpressions used for pushdown; + * @param context a mapping from variable references to column handles used for pushdown * @return a ClpExpression containing either the equivalent KQL query, or the original * expression if it couldn't be translated */ - private ClpExpression handleLike(CallExpression node, Set context) + private ClpExpression handleLike(CallExpression node, Map context) { if (node.getArguments().size() != 2) { throw new PrestoException(CLP_PUSHDOWN_UNSUPPORTED_EXPRESSION, "LIKE operator must have exactly two arguments. Received: " + node); @@ -349,11 +347,11 @@ else if (argument instanceof CallExpression) { * * @param operator the binary operator (e.g., EQUAL, NOT_EQUAL) * @param node the call expression representing the binary operation - * @param context a set of VariableReferenceExpressions used for pushdown; + * @param context a mapping from variable references to column handles used for pushdown * @return a ClpExpression containing either the equivalent KQL query, or the original * expression if it couldn't be translated */ - private ClpExpression handleLogicalBinary(OperatorType operator, CallExpression node, Set context) + private ClpExpression handleLogicalBinary(OperatorType operator, CallExpression node, Map context) { if (node.getArguments().size() != 2) { throw new PrestoException(CLP_PUSHDOWN_UNSUPPORTED_EXPRESSION, @@ -475,14 +473,14 @@ else if (LOGICAL_BINARY_OPS_FILTER.contains(operator) && !(literalType instanceo * @param operator the comparison operator (should be EQUAL) * @param possibleSubstring the left or right expression, possibly a SUBSTR call * @param possibleLiteral the opposite expression, possibly a string constant - * @param context a set of VariableReferenceExpressions used for pushdown; + * @param context a mapping from variable references to column handles used for pushdown * @return an Optional containing a ClpExpression with the equivalent KQL query */ private Optional tryInterpretSubstringEquality( OperatorType operator, RowExpression possibleSubstring, RowExpression possibleLiteral, - Set context) + Map context) { if (!operator.equals(EQUAL)) { return Optional.empty(); @@ -506,10 +504,10 @@ private Optional tryInterpretSubstringEquality( * Parses a SUBSTR(x, start [, length]) call into a SubstrInfo object if valid. * * @param callExpression the call expression to inspect - * @param context a set of VariableReferenceExpressions used for pushdown; + * @param context a mapping from variable references to column handles used for pushdown * @return an Optional containing SubstrInfo if the expression is a valid SUBSTR call */ - private Optional parseSubstringCall(CallExpression callExpression, Set context) + private Optional parseSubstringCall(CallExpression callExpression, Map context) { FunctionMetadata functionMetadata = functionMetadataManager.getFunctionMetadata(callExpression.getFunctionHandle()); String functionName = functionMetadata.getName().getObjectName(); @@ -660,10 +658,10 @@ private Optional parseLengthLiteral(RowExpression lengthExpression, Str * Example: col1 = 5 AND col2 = 'abc' → (col1: 5 AND col2: "abc") * * @param node the AND special form expression - * @param context a set of VariableReferenceExpressions used for pushdown; + * @param context a mapping from variable references to column handles used for pushdown * @return a ClpExpression containing the KQL query and any remaining sub-expressions */ - private ClpExpression handleAnd(SpecialFormExpression node, Set context) + private ClpExpression handleAnd(SpecialFormExpression node, Map context) { StringBuilder metadataQueryBuilder = new StringBuilder(); metadataQueryBuilder.append("("); @@ -719,11 +717,11 @@ else if (!remainingExpressions.isEmpty()) { * Example: col1 = 5 OR col1 = 10 → (col1: 5 OR col1: 10) * * @param node the OR special form expression - * @param context a set of VariableReferenceExpressions used for pushdown; + * @param context a mapping from variable references to column handles used for pushdown * @return a ClpExpression containing either the equivalent KQL query, or the original * expression if it couldn't be fully translated */ - private ClpExpression handleOr(SpecialFormExpression node, Set context) + private ClpExpression handleOr(SpecialFormExpression node, Map context) { StringBuilder metadataQueryBuilder = new StringBuilder(); metadataQueryBuilder.append("("); @@ -764,11 +762,11 @@ private ClpExpression handleOr(SpecialFormExpression node, Setcol1 IN (1, 2, 3) → (col1: 1 OR col1: 2 OR col1: 3) * * @param node the IN special form expression - * @param context a set of VariableReferenceExpressions used for pushdown; + * @param context a mapping from variable references to column handles used for pushdown * @return a ClpExpression containing either the equivalent KQL query, or the original * expression if it couldn't be translated */ - private ClpExpression handleIn(SpecialFormExpression node, Set context) + private ClpExpression handleIn(SpecialFormExpression node, Map context) { ClpExpression variable = node.getArguments().get(0).accept(this, context); if (!variable.getPushDownExpression().isPresent()) { @@ -803,11 +801,11 @@ private ClpExpression handleIn(SpecialFormExpression node, Setcol1 IS NULL → NOT col1: * * * @param node the IS_NULL special form expression - * @param context a set of VariableReferenceExpressions used for pushdown; + * @param context a mapping from variable references to column handles used for pushdown * @return a ClpExpression containing either the equivalent KQL query, or the original * expression if it couldn't be translated */ - private ClpExpression handleIsNull(SpecialFormExpression node, Set context) + private ClpExpression handleIsNull(SpecialFormExpression node, Map context) { if (node.getArguments().size() != 1) { throw new PrestoException(CLP_PUSHDOWN_UNSUPPORTED_EXPRESSION, @@ -832,11 +830,11 @@ private ClpExpression handleIsNull(SpecialFormExpression node, Set context) + private ClpExpression handleDereference(RowExpression expression, Map context) { if (expression instanceof VariableReferenceExpression) { return expression.accept(this, context); 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 index 0e67e5b76c467..7c907fc1d3191 100644 --- 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 @@ -29,10 +29,9 @@ import com.facebook.presto.spi.relation.RowExpression; import com.facebook.presto.spi.relation.VariableReferenceExpression; -import java.util.HashSet; +import java.util.HashMap; import java.util.Map; import java.util.Optional; -import java.util.Set; import static com.facebook.presto.plugin.clp.ClpConnectorFactory.CONNECTOR_NAME; import static com.facebook.presto.spi.ConnectorPlanRewriter.rewriteWith; @@ -77,18 +76,16 @@ public PlanNode visitFilter(FilterNode node, RewriteContext context) } TableScanNode tableScanNode = (TableScanNode) node.getSource(); - Map assignments = tableScanNode.getAssignments(); + Map assignments = new HashMap<>(tableScanNode.getAssignments()); TableHandle tableHandle = tableScanNode.getTable(); ClpTableHandle clpTableHandle = (ClpTableHandle) tableHandle.getConnectorHandle(); String scope = CONNECTOR_NAME + "." + clpTableHandle.getSchemaTableName().toString(); - Set clpUdfVariablesInFilterNode = new HashSet<>(); ClpExpression clpExpression = node.getPredicate().accept( new ClpFilterToKqlConverter( functionResolution, functionManager, - assignments, metadataFilterProvider.getColumnNames(scope)), - clpUdfVariablesInFilterNode); + assignments); Optional kqlQuery = clpExpression.getPushDownExpression(); Optional metadataSqlQuery = clpExpression.getMetadataSqlQuery(); Optional remainingPredicate = clpExpression.getRemainingExpression(); diff --git a/presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpFilterToKql.java b/presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpFilterToKql.java index 6fe74105da970..ff08d026be7ef 100644 --- a/presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpFilterToKql.java +++ b/presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpFilterToKql.java @@ -13,12 +13,15 @@ */ package com.facebook.presto.plugin.clp; +import com.facebook.presto.spi.ColumnHandle; import com.facebook.presto.spi.relation.RowExpression; import com.facebook.presto.spi.relation.VariableReferenceExpression; import com.google.common.collect.ImmutableSet; import org.testng.annotations.Test; +import java.util.HashMap; import java.util.HashSet; +import java.util.Map; import java.util.Optional; import java.util.Set; @@ -296,13 +299,13 @@ private ClpExpression tryPushDown( Set clpUdfVariables) { RowExpression pushDownExpression = getRowExpression(sqlExpression, sessionHolder); + Map assignments = new HashMap<>(variableToColumnHandleMap); return pushDownExpression.accept( new ClpFilterToKqlConverter( standardFunctionResolution, functionAndTypeManager, - variableToColumnHandleMap, metadataFilterColumns), - clpUdfVariables); + assignments); } private void testFilter( From 42028de86db7ad0f9683741785b902b726adf7f0 Mon Sep 17 00:00:00 2001 From: wraymo Date: Wed, 16 Jul 2025 09:34:08 -0400 Subject: [PATCH 3/4] address review comments --- .../presto/plugin/clp/TestClpFilterToKql.java | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpFilterToKql.java b/presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpFilterToKql.java index ff08d026be7ef..0bd8378ba5362 100644 --- a/presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpFilterToKql.java +++ b/presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpFilterToKql.java @@ -273,16 +273,14 @@ public void testMetadataSqlGeneration() private void testPushDown(SessionHolder sessionHolder, String sql, String expectedKql, String expectedRemaining) { - HashSet clpUdfVariables = new HashSet<>(); - ClpExpression clpExpression = tryPushDown(sql, sessionHolder, ImmutableSet.of(), clpUdfVariables); - testFilter(clpExpression, expectedKql, expectedRemaining, clpUdfVariables, sessionHolder); + ClpExpression clpExpression = tryPushDown(sql, sessionHolder, ImmutableSet.of()); + testFilter(clpExpression, expectedKql, expectedRemaining, sessionHolder); } private void testPushDown(SessionHolder sessionHolder, String sql, String expectedKql, String expectedMetadataSqlQuery, Set metadataFilterColumns) { - HashSet clpUdfVariables = new HashSet<>(); - ClpExpression clpExpression = tryPushDown(sql, sessionHolder, metadataFilterColumns, clpUdfVariables); - testFilter(clpExpression, expectedKql, null, clpUdfVariables, sessionHolder); + ClpExpression clpExpression = tryPushDown(sql, sessionHolder, metadataFilterColumns); + testFilter(clpExpression, expectedKql, null, sessionHolder); if (expectedMetadataSqlQuery != null) { assertTrue(clpExpression.getMetadataSqlQuery().isPresent()); assertEquals(clpExpression.getMetadataSqlQuery().get(), expectedMetadataSqlQuery); @@ -295,8 +293,7 @@ private void testPushDown(SessionHolder sessionHolder, String sql, String expect private ClpExpression tryPushDown( String sqlExpression, SessionHolder sessionHolder, - Set metadataFilterColumns, - Set clpUdfVariables) + Set metadataFilterColumns,) { RowExpression pushDownExpression = getRowExpression(sqlExpression, sessionHolder); Map assignments = new HashMap<>(variableToColumnHandleMap); @@ -312,7 +309,6 @@ private void testFilter( ClpExpression clpExpression, String expectedKqlExpression, String expectedRemainingExpression, - Set clpUdfVariables, SessionHolder sessionHolder) { Optional kqlExpression = clpExpression.getPushDownExpression(); From 61f828ce8fb1f1fcd06c70ff9153f369aebb4a60 Mon Sep 17 00:00:00 2001 From: wraymo Date: Wed, 16 Jul 2025 09:42:48 -0400 Subject: [PATCH 4/4] fix --- .../presto/plugin/clp/TestClpFilterToKql.java | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpFilterToKql.java b/presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpFilterToKql.java index 0bd8378ba5362..9e9e9db13a665 100644 --- a/presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpFilterToKql.java +++ b/presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpFilterToKql.java @@ -20,7 +20,6 @@ import org.testng.annotations.Test; import java.util.HashMap; -import java.util.HashSet; import java.util.Map; import java.util.Optional; import java.util.Set; @@ -290,10 +289,7 @@ private void testPushDown(SessionHolder sessionHolder, String sql, String expect } } - private ClpExpression tryPushDown( - String sqlExpression, - SessionHolder sessionHolder, - Set metadataFilterColumns,) + private ClpExpression tryPushDown(String sqlExpression, SessionHolder sessionHolder, Set metadataFilterColumns) { RowExpression pushDownExpression = getRowExpression(sqlExpression, sessionHolder); Map assignments = new HashMap<>(variableToColumnHandleMap); @@ -305,11 +301,7 @@ private ClpExpression tryPushDown( assignments); } - private void testFilter( - ClpExpression clpExpression, - String expectedKqlExpression, - String expectedRemainingExpression, - SessionHolder sessionHolder) + private void testFilter(ClpExpression clpExpression, String expectedKqlExpression, String expectedRemainingExpression, SessionHolder sessionHolder) { Optional kqlExpression = clpExpression.getPushDownExpression(); Optional remainingExpression = clpExpression.getRemainingExpression();