feat: Set context in ClpFilterToKqlConverter to track VariableReferenceExpression. #38
Conversation
WalkthroughThe changes update the Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Method
participant Converter as ClpFilterToKqlConverter
participant Expr as RowExpression
Test->>Converter: Create local assignments map copy
Test->>Expr: Prepare RowExpression
Test->>Converter: Expr.accept(Converter, assignmentsMap)
loop Visitor Traversal
Converter->>Expr: accept(Converter, assignmentsMap)
Expr-->>Converter: visitX(..., assignmentsMap)
Converter->>assignmentsMap: (use or propagate context)
end
sequenceDiagram
participant PlanOpt as ClpPlanOptimizer
participant ContextMap as Map<VariableReferenceExpression, ColumnHandle>
participant Converter as ClpFilterToKqlConverter
participant Predicate as RowExpression
PlanOpt->>ContextMap: Create HashMap copy of assignments
PlanOpt->>Converter: new ClpFilterToKqlConverter(...)
PlanOpt->>Predicate: getPredicate()
PlanOpt->>Converter: Predicate.accept(Converter, ContextMap)
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpFilterToKql.java (1)🔇 Additional comments (3)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpFilterToKql.java (1)
273-332: Add test coverage for CLP UDF variable collectionThe tests create and pass the
clpUdfVariablesset but never verify its contents. Since the PR's main objective is to track variables from CLP_GET_* functions, consider adding test cases that:
- Include CLP_GET_* functions in the filter expressions
- Assert that the expected variables are collected in the set
- Verify the set remains empty when no CLP UDF functions are present
This would ensure the variable tracking functionality works as intended.
presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpFilterToKqlConverter.java (1)
98-183: Context parameter is passed but never populatedThe
Set<VariableReferenceExpression>context parameter is now passed through all visitor methods, but there's no code that actually adds variables to this set. According to the PR objectives, this set should track variables from CLP_GET_* functions.Should there be logic in
visitCallor other methods to detect CLP UDF function calls and add their output variables to the context set?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpFilterToKqlConverter.java(23 hunks)presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpPlanOptimizer.java(2 hunks)presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpFilterToKql.java(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: test (17.0.13, :presto-tests -P ci-only-distributed-non-hash-gen)
- GitHub Check: test (8.0.442, :presto-main)
- GitHub Check: test (8.0.442, :presto-tests -P ci-only-aggregation-queries)
- GitHub Check: test (8.0.442, :presto-tests -P ci-only-tpch-distributed-queries)
- GitHub Check: test (8.0.442, :presto-tests -P ci-only-distributed-queries)
- GitHub Check: test (8.0.442, :presto-tests -P presto-tests-general)
- GitHub Check: test (8.0.442, :presto-tests -P ci-only-resource-manager)
- GitHub Check: test (8.0.442, :presto-tests -P presto-tests-execution-memory)
- GitHub Check: test (8.0.442, :presto-tests -P ci-only-distributed-non-hash-gen)
- GitHub Check: maven-checks (17.0.13)
- GitHub Check: maven-checks (8.0.442)
🔇 Additional comments (2)
presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpPlanOptimizer.java (1)
84-91: Missing usage of collected CLP UDF variablesThe
clpUdfVariablesInFilterNodeset is created and passed to the converter, but it's never used afterwards. According to the PR objectives, these variables should be tracked so they can be included as outputs in the correspondingTableScanNode. However, I don't see any code that adds these collected variables to the table scan node outputs.Should there be additional logic after line 91 to handle the collected variables?
presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpFilterToKqlConverter.java (1)
365-373: Good refactoring to use Optional for substring methodsThe change to return
Optional<ClpExpression>in substring-related methods is a good improvement. It makes the API clearer by explicitly representing the absence of a valid interpretation rather than returning emptyClpExpressioninstances.Also applies to: 481-503, 556-596
| @Override | ||
| public ClpExpression visitCall(CallExpression node, Void context) | ||
| public ClpExpression visitCall(CallExpression node, Set<VariableReferenceExpression> 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()); | ||
| Optional<OperatorType> operatorTypeOptional = functionMetadata.getOperatorType(); | ||
| 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); | ||
| } | ||
| } | ||
|
|
||
| return new ClpExpression(node); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Missing CLP UDF function detection logic
The visitCall method should detect CLP_GET_* functions and track their output variables in the context set, but there's no such logic implemented. Without this, the context parameter serves no purpose.
Consider adding logic to check if the function name starts with "CLP_GET_" and add the corresponding variable to the context set.
🏁 Script executed:
#!/bin/bash
# Search for CLP_GET functions or references in the codebase
echo "Searching for CLP_GET function references..."
rg -i "clp_get" --type java -A 3 -B 3
# Search for any UDF-related code that might give hints about expected implementation
echo -e "\nSearching for UDF-related code..."
rg -i "udf|user.defined.function" --type java -g "*Clp*.java" -A 2 -B 2Length of output: 5707
Add CLP_GET_ UDF detection to ClpFilterToKqlConverter*
The visitCall method never checks for CLP_GET_* functions, so the clpUdfVariables set remains empty and your push-down logic cannot track UDF outputs. We need to detect any function whose name starts with CLP_GET_ and add its output variable to the context.
Locations to update:
presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpFilterToKqlConverter.java, methodvisitCall(CallExpression node, Set<VariableReferenceExpression> context)
Suggested diff:
if (BETWEEN == operatorType) {
return handleBetween(node, context);
}
}
+ // ≫ Detect CLP_GET_* UDFs and collect their output variable
+ String functionName = node.getFunctionHandle()
+ .getSignature()
+ .getName()
+ .toUpperCase(Locale.ROOT);
+ if (functionName.startsWith("CLP_GET_")) {
+ // Assuming the UDF returns a single VariableReferenceExpression argument
+ Expression arg0 = node.getArguments().get(0);
+ if (arg0 instanceof VariableReferenceExpression) {
+ context.add((VariableReferenceExpression) arg0);
+ }
+ return new ClpExpression(node);
+ }
+
return new ClpExpression(node);Don’t forget to add:
import java.util.Locale;With this in place, clpUdfVariables will be populated and TestClpFilterToKql will pass as intended.
🤖 Prompt for AI Agents
In
presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpFilterToKqlConverter.java
around lines 120 to 145, the visitCall method does not detect functions with
names starting with "CLP_GET_", causing the clpUdfVariables set to remain empty
and preventing tracking of UDF outputs. To fix this, add a check in visitCall to
detect if the function name starts with "CLP_GET_" (case-insensitive), and if
so, add the output variable of the node to the context set. Also, add the import
statement for java.util.Locale to support case-insensitive checks.
wraymo
changed the title
presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpFilterToKqlConverter.java
Show resolved
Hide resolved
presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpPlanOptimizer.java
Show resolved
Hide resolved
presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpFilterToKql.java
Outdated
Show resolved
Hide resolved
presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpFilterToKql.java
Outdated
Show resolved
Hide resolved
presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpFilterToKql.java
Outdated
Show resolved
Hide resolved
presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpFilterToKql.java
Outdated
Show resolved
Hide resolved
presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpFilterToKql.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpFilterToKql.java (1)
299-299: Consider using ImmutableMap for better immutability.Based on the retrieved learnings and past review comments, prefer using Guava's Immutable collections when possible. Since this is a copy operation that doesn't appear to require mutation within the test context, consider using
ImmutableMap.copyOf().-Map<VariableReferenceExpression, ColumnHandle> assignments = new HashMap<>(variableToColumnHandleMap); +Map<VariableReferenceExpression, ColumnHandle> assignments = ImmutableMap.copyOf(variableToColumnHandleMap);Note: You'll need to add the import for
ImmutableMap:+import com.google.common.collect.ImmutableMap;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpFilterToKql.java(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpFilterToKql.java (1)
Learnt from: anlowee
PR: y-scope/presto#0
File: :0-0
Timestamp: 2025-06-19T08:41:46.288Z
Learning: In the y-scope/presto repository, prefer using Guava's Immutable collections over regular collections when possible.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: test (17.0.13, :presto-tests -P ci-only-aggregation-queries)
- GitHub Check: test (17.0.13, :presto-main-base)
- GitHub Check: test (17.0.13, :presto-tests -P ci-only-plan-determinism)
- GitHub Check: test (17.0.13, :presto-tests -P ci-only-resource-manager)
- GitHub Check: test (17.0.13, :presto-tests -P ci-only-distributed-queries)
- GitHub Check: test (17.0.13, :presto-tests -P ci-only-tpch-distributed-queries)
- GitHub Check: test (8.0.442, :presto-tests -P ci-only-tpch-distributed-queries)
- GitHub Check: test (17.0.13, :presto-tests -P ci-only-distributed-non-hash-gen)
- GitHub Check: test (17.0.13, :presto-tests -P presto-tests-general)
- GitHub Check: test (17.0.13, :presto-tests -P presto-tests-execution-memory)
- GitHub Check: test (8.0.442, :presto-tests -P ci-only-plan-determinism)
- GitHub Check: test (8.0.442, :presto-main)
- GitHub Check: test (8.0.442, :presto-tests -P ci-only-resource-manager)
- GitHub Check: test (8.0.442, :presto-main-base)
- GitHub Check: test (8.0.442, :presto-tests -P ci-only-local-queries)
- GitHub Check: test (8.0.442, :presto-tests -P ci-only-distributed-non-hash-gen)
- GitHub Check: test (8.0.442, :presto-tests -P presto-tests-general)
- GitHub Check: test (8.0.442, :presto-tests -P presto-tests-execution-memory)
- GitHub Check: maven-checks (8.0.442)
- GitHub Check: maven-checks (17.0.13)
🔇 Additional comments (2)
presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpFilterToKql.java (2)
293-296: LGTM! Method signature updated correctly.The method signature properly includes the
metadataFilterColumnsparameter, which aligns with the PR objectives.
305-305: LGTM! Correctly passes assignments to converter.The change properly passes the assignments map as context to the
ClpFilterToKqlConverter, which aligns with the PR objective to trackVariableReferenceExpressionthrough the conversion process.
presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpFilterToKql.java
Outdated
Show resolved
Hide resolved
kirkrodrigues
changed the title
kirkrodrigues
merged commit 787783b
into
y-scope:release-0.293-clp-connector
3 participants
Description
This is a split of #31. When implementing CLP UDFs, we need to transform each
CLP_GET_*function into aVariableReferenceExpression. During this transformation, it's important to track all variables derived fromCLP_GET_*so that they can later be included as outputs in the correspondingTableScanNode.Checklist
breaking change.
Validation performed
All unit tests passed
Summary by CodeRabbit
Summary by CodeRabbit
Refactor
Tests