Skip to content

feat(presto-clp): Add a plan optimizer to push down supported filters to CLP. #16

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Jun 24, 2025
Merged

feat(presto-clp): Add a plan optimizer to push down supported filters to CLP. #16

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e94e218
add plan optimizer for clp connector
wraymo Jun 19, 2025
b0fc053
apply review suggestions
wraymo Jun 20, 2025
b6ee2ea
fix inconsistent variable name and remove Optional from test cases
wraymo Jun 20, 2025
544ab52
apply review suggestions
wraymo Jun 23, 2025
7dcc7fc
rename definition to pushDownExpression in ClpExpression
wraymo Jun 23, 2025
636d35e
apply review suggestion
wraymo Jun 24, 2025
537c424
fix a name in comments
wraymo Jun 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 76 additions & 0 deletions presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpExpression.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
/*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.facebook.presto.plugin.clp;

import com.facebook.presto.spi.relation.RowExpression;

import java.util.Optional;

/**
* Represents the result of converting a Presto RowExpression into a CLP-compatible KQL query. In
* every case, `kqlQuery` represents the part of the RowExpression that could be converted to a
Copy link

@anlowee anlowee Jun 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to update this docstring? We have renamed the kqlQuery to pushDownExpression

Copy link
Member

@kirkrodrigues kirkrodrigues Jun 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wraymo

* KQL expression, and `remainingExpression` represents the part that could not be converted.
*/
public class ClpExpression
{
// Optional KQL query or column name representing the fully or partially translatable part of the expression.
private final Optional<String> pushDownExpression;

// The remaining (non-translatable) portion of the RowExpression, if any.
private final Optional<RowExpression> remainingExpression;

public ClpExpression(String pushDownExpression, RowExpression remainingExpression)
{
this.pushDownExpression = Optional.ofNullable(pushDownExpression);
this.remainingExpression = Optional.ofNullable(remainingExpression);
}

/**
* Creates an empty ClpExpression with neither pushdown nor remaining expressions.
*/
public ClpExpression()
{
this(null, null);
}

/**
* Creates a ClpExpression from a fully translatable KQL query or column name.
*
* @param pushDownExpression
*/
public ClpExpression(String pushDownExpression)
{
this(pushDownExpression, null);
}
Comment on lines +48 to +56
Copy link

@coderabbitai coderabbitai bot Jun 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Add description to @param tag.

The JavaDoc has an empty @param tag without description.

     /**
      * Creates a ClpExpression from a fully translatable KQL query or column name.
      *
-     * @param pushDownExpression
+     * @param pushDownExpression the KQL query string or column name
      */
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
/**
* Creates a ClpExpression from a fully translatable KQL query or column name.
*
* @param pushDownExpression
*/
public ClpExpression(String pushDownExpression)
{
this(pushDownExpression, null);
}
/**
* Creates a ClpExpression from a fully translatable KQL query or column name.
*
* @param pushDownExpression the KQL query string or column name
*/
public ClpExpression(String pushDownExpression)
{
this(pushDownExpression, null);
}
🤖 Prompt for AI Agents 
In presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpExpression.java
around lines 47 to 55, the JavaDoc for the constructor has an empty @param tag
for pushDownExpression. Add a clear description explaining what
pushDownExpression represents, such as "the fully translatable KQL query or
column name to create the ClpExpression from."


/**
* Creates a ClpExpression from a non-translatable RowExpression.
*
* @param remainingExpression
*/
public ClpExpression(RowExpression remainingExpression)
{
this(null, remainingExpression);
}
Comment on lines +58 to +66
Copy link

@coderabbitai coderabbitai bot Jun 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Add description to @param tag.

The JavaDoc has an empty @param tag without description.

     /**
      * Creates a ClpExpression from a non-translatable RowExpression.
      *
-     * @param remainingExpression
+     * @param remainingExpression the non-translatable RowExpression
      */
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
/**
* Creates a ClpExpression from a non-translatable RowExpression.
*
* @param remainingExpression
*/
public ClpExpression(RowExpression remainingExpression)
{
this(null, remainingExpression);
}
/**
* Creates a ClpExpression from a non-translatable RowExpression.
*
* @param remainingExpression the non-translatable RowExpression
*/
public ClpExpression(RowExpression remainingExpression)
{
this(null, remainingExpression);
}
🤖 Prompt for AI Agents 
In presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpExpression.java
around lines 57 to 65, the JavaDoc for the constructor has an empty @param tag
for remainingExpression. Add a clear description to the @param tag explaining
what remainingExpression represents, such as "the non-translatable RowExpression
to create the ClpExpression from."


public Optional<String> getPushDownExpression()
{
return pushDownExpression;
}

public Optional<RowExpression> getRemainingExpression()
{
return remainingExpression;
}
Comment on lines +68 to +76
Copy link

@coderabbitai coderabbitai bot Jun 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Consider adding JavaDoc for getter methods.

While the getter methods are self-explanatory, adding JavaDoc would improve consistency with the rest of the class and provide clarity about the Optional return types.

+    /**
+     * Returns the KQL query or column name that can be pushed down, if any.
+     *
+     * @return Optional containing the pushdown expression, or empty if none exists
+     */
     public Optional<String> getPushDownExpression()
     {
         return pushDownExpression;
     }

+    /**
+     * Returns the remaining non-translatable RowExpression, if any.
+     *
+     * @return Optional containing the remaining expression, or empty if fully translatable
+     */
     public Optional<RowExpression> getRemainingExpression()
     {
         return remainingExpression;
     }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
public Optional<String> getPushDownExpression()
{
return pushDownExpression;
}
public Optional<RowExpression> getRemainingExpression()
{
return remainingExpression;
}
/**
* Returns the KQL query or column name that can be pushed down, if any.
*
* @return Optional containing the pushdown expression, or empty if none exists
*/
public Optional<String> getPushDownExpression()
{
return pushDownExpression;
}
/**
* Returns the remaining non-translatable RowExpression, if any.
*
* @return Optional containing the remaining expression, or empty if fully translatable
*/
public Optional<RowExpression> getRemainingExpression()
{
return remainingExpression;
}
🤖 Prompt for AI Agents 
In presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpExpression.java
around lines 68 to 76, the getter methods getPushDownExpression and
getRemainingExpression lack JavaDoc comments. Add concise JavaDoc comments above
each method explaining what the method returns, emphasizing that the return type
is Optional and what it may contain or represent, to maintain consistency and
improve clarity.

}
Comment on lines +26 to +77
Copy link

@coderabbitai coderabbitai bot Jun 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Consider adding standard object methods for better usability.

The class lacks equals(), hashCode(), and toString() methods, which are typically expected for data classes and would improve testability and debugging capabilities.

+    @Override
+    public boolean equals(Object obj)
+    {
+        if (this == obj) {
+            return true;
+        }
+        if (obj == null || getClass() != obj.getClass()) {
+            return false;
+        }
+        ClpExpression that = (ClpExpression) obj;
+        return pushDownExpression.equals(that.pushDownExpression) &&
+               remainingExpression.equals(that.remainingExpression);
+    }
+
+    @Override
+    public int hashCode()
+    {
+        return java.util.Objects.hash(pushDownExpression, remainingExpression);
+    }
+
+    @Override
+    public String toString()
+    {
+        return "ClpExpression{" +
+               "pushDownExpression=" + pushDownExpression +
+               ", remainingExpression=" + remainingExpression +
+               '}';
+    }
🤖 Prompt for AI Agents 
In presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpExpression.java
between lines 26 and 77, the class ClpExpression lacks the standard methods
equals(), hashCode(), and toString(). To fix this, implement these methods to
compare instances based on pushDownExpression and remainingExpression fields,
generate consistent hash codes from these fields, and provide a clear string
representation of the object including its field values. This will improve
usability, testability, and debugging.

Loading
Loading