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 1 commit
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
70 changes: 70 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,70 @@
/*
* 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.
* There are three possible cases:
* 1. The entire RowExpression is convertible to KQL: `definition` is set, `remainingExpression` is empty.
* 2. Part of the RowExpression is convertible: the KQL part is stored in `definition`,
* and the remaining untranslatable part is stored in `remainingExpression`.
* 3. None of the expression is convertible: the full RowExpression is stored in `remainingExpression`,
* and `definition` is empty.
*/
public class ClpExpression
{
// Optional KQL query string representing the fully or partially translatable part of the expression.
private final Optional<String> definition;

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

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

// Creates an empty ClpExpression (no KQL definition, no remaining expression).
public ClpExpression()
{
this (null, null);
}

// Creates a ClpExpression from a fully translatable KQL string.
public ClpExpression(String definition)
{
this(definition, null);
}

// Creates a ClpExpression from a non-translatable RowExpression.
public ClpExpression(RowExpression remainingExpression)
{
this(null, remainingExpression);
}
Copy link

@coderabbitai coderabbitai bot Jun 20, 2025

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Consider improving constructor parameter validation.

The constructors currently accept null parameters and wrap them in Optional, which could lead to confusion. Consider making the intent clearer by either:

  1. Using explicit null checks with meaningful parameter names
  2. Providing static factory methods with clearer semantics
// Alternative approach with static factory methods
+public static ClpExpression fullyTranslatable(String definition) {
+    return new ClpExpression(requireNonNull(definition), null);
+}
+
+public static ClpExpression partiallyTranslatable(String definition, RowExpression remaining) {
+    return new ClpExpression(requireNonNull(definition), requireNonNull(remaining));
+}
+
+public static ClpExpression nonTranslatable(RowExpression remaining) {
+    return new ClpExpression(null, requireNonNull(remaining));
+}
📝 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 ClpExpression(String definition, RowExpression remainingExpression)
{
this.definition = Optional.ofNullable(definition);
this.remainingExpression = Optional.ofNullable(remainingExpression);
}
// Creates an empty ClpExpression (no KQL definition, no remaining expression).
public ClpExpression()
{
this (null, null);
}
// Creates a ClpExpression from a fully translatable KQL string.
public ClpExpression(String definition)
{
this(definition, null);
}
// Creates a ClpExpression from a non-translatable RowExpression.
public ClpExpression(RowExpression remainingExpression)
{
this(null, remainingExpression);
}
public ClpExpression(String definition, RowExpression remainingExpression)
{
this.definition = Optional.ofNullable(definition);
this.remainingExpression = Optional.ofNullable(remainingExpression);
}
// Creates an empty ClpExpression (no KQL definition, no remaining expression).
public ClpExpression()
{
this(null, null);
}
// Creates a ClpExpression from a fully translatable KQL string.
public ClpExpression(String definition)
{
this(definition, null);
}
// Creates a ClpExpression from a non-translatable RowExpression.
public ClpExpression(RowExpression remainingExpression)
{
this(null, remainingExpression);
}
// Alternative approach with static factory methods
public static ClpExpression fullyTranslatable(String definition) {
return new ClpExpression(requireNonNull(definition), null);
}
public static ClpExpression partiallyTranslatable(String definition, RowExpression remaining) {
return new ClpExpression(requireNonNull(definition), requireNonNull(remaining));
}
public static ClpExpression nonTranslatable(RowExpression remaining) {
return new ClpExpression(null, requireNonNull(remaining));
}
🤖 Prompt for AI Agents 
In presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpExpression.java
around lines 37 to 59, the constructors accept null parameters and wrap them in
Optional, which can be confusing. To fix this, add explicit null checks in the
constructors to validate parameters and throw IllegalArgumentException if
invalid. Alternatively, refactor by making constructors private and provide
static factory methods with descriptive names for creating instances with
definition, remainingExpression, or empty, to clarify intent and avoid null
usage.


public Optional<String> getDefinition()
{
return definition;
}

public Optional<RowExpression> getRemainingExpression()
{
return remainingExpression;
}
}
Loading
Loading