Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
10 changes: 10 additions & 0 deletions presto-clp/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,16 @@
<scope>provided</scope>
</dependency>

<dependency>
<groupId>com.fasterxml.jackson.core</groupId>
<artifactId>jackson-core</artifactId>
</dependency>

<dependency>
<groupId>com.fasterxml.jackson.core</groupId>
<artifactId>jackson-databind</artifactId>
</dependency>

<dependency>
<groupId>com.facebook.presto</groupId>
<artifactId>presto-spi</artifactId>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ public class ClpConfig
private String metadataDbName;
private String metadataDbUser;
private String metadataDbPassword;
private String metadataFilterConfig;
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

LGTM! Consistent configuration property implementation.

The new metadataFilterConfig property follows the established pattern with proper getter/setter methods and @config annotation.

Consider adding validation for the metadata filter config path, similar to how metadataTablePrefix is validated:

@Config("clp.metadata-filter-config")
public ClpConfig setMetadataFilterConfig(String metadataFilterConfig)
{
+   if (metadataFilterConfig != null && !metadataFilterConfig.trim().isEmpty()) {
+       // Validate file path format or existence if needed
+   }
    this.metadataFilterConfig = metadataFilterConfig;
    return this;
}

Also applies to: 111-121

🤖 Prompt for AI Agents
In presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpConfig.java around
lines 32 and 111-121, the metadataFilterConfig property lacks validation similar
to metadataTablePrefix. Add validation logic in the setter method for
metadataFilterConfig to ensure the config path meets expected criteria, throwing
an appropriate exception if invalid. This will maintain consistency and prevent
misconfiguration.

private String metadataTablePrefix;
private long metadataRefreshInterval = 60;
private long metadataExpireInterval = 600;
Expand Down Expand Up @@ -107,6 +108,18 @@ public ClpConfig setMetadataDbPassword(String metadataDbPassword)
return this;
}

public String getMetadataFilterConfig()
{
return metadataFilterConfig;
}

@Config("clp.metadata-filter-config")
public ClpConfig setMetadataFilterConfig(String metadataFilterConfig)
{
this.metadataFilterConfig = metadataFilterConfig;
return this;
}

public String getMetadataTablePrefix()
{
return metadataTablePrefix;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,12 @@
public class ClpConnectorFactory
implements ConnectorFactory
{
public static final String CONNECTOR_NAME = "clp";

@Override
public String getName()
{
return "clp";
return CONNECTOR_NAME;
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import com.facebook.presto.spi.ErrorCodeSupplier;

import static com.facebook.presto.common.ErrorType.EXTERNAL;
import static com.facebook.presto.common.ErrorType.USER_ERROR;

public enum ClpErrorCode
implements ErrorCodeSupplier
Expand All @@ -26,7 +27,10 @@ public enum ClpErrorCode
CLP_UNSUPPORTED_METADATA_SOURCE(1, EXTERNAL),
CLP_UNSUPPORTED_SPLIT_SOURCE(2, EXTERNAL),
CLP_UNSUPPORTED_TYPE(3, EXTERNAL),
CLP_UNSUPPORTED_CONFIG_OPTION(4, EXTERNAL);
CLP_UNSUPPORTED_CONFIG_OPTION(4, EXTERNAL),

CLP_METADATA_FILTER_CONFIG_NOT_FOUND(10, USER_ERROR),
CLP_MANDATORY_METADATA_FILTER_NOT_VALID(11, USER_ERROR);

private final ErrorCode errorCode;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,15 @@ public class ClpExpression
{
// Optional KQL query or column name representing the fully or partially translatable part of the expression.
private final Optional<String> pushDownExpression;

// Optional SQL string extracted from the pushDownExpression, which is only made of given metadata columns.
private final Optional<String> metadataSql;
// The remaining (non-translatable) portion of the RowExpression, if any.
private final Optional<RowExpression> remainingExpression;

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

Expand All @@ -42,7 +44,7 @@ public ClpExpression(String pushDownExpression, RowExpression remainingExpressio
*/
public ClpExpression()
{
this(null, null);
this(null, null, null);
}

/**
Expand All @@ -52,7 +54,18 @@ public ClpExpression()
*/
public ClpExpression(String pushDownExpression)
{
this(pushDownExpression, null);
this(pushDownExpression, null, null);
}

/**
* Creates a ClpExpression from a fully translatable KQL string and a metadata SQL string.
*
* @param pushDownExpression
* @param metadataSql
*/
public ClpExpression(String pushDownExpression, String metadataSql)
{
this(pushDownExpression, metadataSql, null);
}

/**
Expand All @@ -62,14 +75,19 @@ public ClpExpression(String pushDownExpression)
*/
public ClpExpression(RowExpression remainingExpression)
{
this(null, remainingExpression);
this(null, null, remainingExpression);
}

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

public Optional<String> getMetadataSql()
{
return metadataSql;
}

public Optional<RowExpression> getRemainingExpression()
{
return remainingExpression;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
import java.util.Optional;
import java.util.Set;

import static com.facebook.presto.common.function.OperatorType.BETWEEN;
import static com.facebook.presto.common.function.OperatorType.EQUAL;
import static com.facebook.presto.common.function.OperatorType.GREATER_THAN;
import static com.facebook.presto.common.function.OperatorType.GREATER_THAN_OR_EQUAL;
Expand All @@ -49,6 +50,8 @@
import static com.facebook.presto.common.function.OperatorType.flip;
import static com.facebook.presto.common.type.BooleanType.BOOLEAN;
import static com.facebook.presto.plugin.clp.ClpErrorCode.CLP_PUSHDOWN_UNSUPPORTED_EXPRESSION;
import static com.facebook.presto.plugin.clp.ClpUtils.escapeKqlSpecialCharsForStringValue;
import static com.facebook.presto.plugin.clp.ClpUtils.isNumericType;
import static com.facebook.presto.spi.relation.SpecialFormExpression.Form.AND;
import static java.lang.Integer.parseInt;
import static java.lang.String.format;
Expand Down Expand Up @@ -84,15 +87,18 @@ public class ClpFilterToKqlConverter
private final StandardFunctionResolution standardFunctionResolution;
private final FunctionMetadataManager functionMetadataManager;
private final Map<VariableReferenceExpression, ColumnHandle> assignments;
private final Set<String> metadataFilterColumns;

public ClpFilterToKqlConverter(
StandardFunctionResolution standardFunctionResolution,
FunctionMetadataManager functionMetadataManager,
Map<VariableReferenceExpression, ColumnHandle> assignments)
Map<VariableReferenceExpression, ColumnHandle> assignments,
Set<String> 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
Expand All @@ -114,11 +120,69 @@ public ClpExpression visitCall(CallExpression node, Void context)
if (operatorType.isComparisonOperator() && operatorType != IS_DISTINCT_FROM) {
return handleLogicalBinary(operatorType, node);
}
if (BETWEEN == operatorType) {
return handleBetween(node);
}
}

return new ClpExpression(node);
}

/**
* Handles the <code>BETWEEN</code> expression. All arguments must be of numeric types.
* <p></p>
* The translation is only performed if:
* - The first argument is a variable reference expression.
* - The second and third arguments are constant expressions.
*
* <p></p>
* Example: <code>col1 BETWEEN 0 AND 5</code> → <code>col1 >= 0 AND col1 <= 5</code>
*
* @param node the {@code BETWEEN} call expression
* @return a ClpExpression containing either the equivalent KQL query, or the original
* expression if it couldn't be translated
*/
private ClpExpression handleBetween(CallExpression node)
{
if (node.getArguments().size() != 3) {
throw new PrestoException(CLP_PUSHDOWN_UNSUPPORTED_EXPRESSION,
"BETWEEN operator must have exactly three arguments. Received: " + node);
}
if (!(node.getArguments().get(0) instanceof VariableReferenceExpression)
|| !(node.getArguments().get(1) instanceof ConstantExpression)
|| !(node.getArguments().get(2) instanceof ConstantExpression)) {
return new ClpExpression(node);
}
if (!isNumericType(node.getArguments().get(0).getType(), true)
|| !isNumericType(node.getArguments().get(1).getType(), false)
|| !isNumericType(node.getArguments().get(2).getType(), false)) {
return new ClpExpression(node);
}
Optional<String> variableReferencePushDownExpression = node.getArguments().get(0).accept(this, null).getPushDownExpression();
if (!variableReferencePushDownExpression.isPresent()) {
return new ClpExpression(node);
}

String lowerBoundConstantPushDownExpression = getLiteralString((ConstantExpression) node.getArguments().get(1));
String upperBoundConstantPushDownExpression = getLiteralString((ConstantExpression) node.getArguments().get(2));
String metadataSql = null;
String kql = format(
"%s >= %s AND %s <= %s",
variableReferencePushDownExpression.get(),
lowerBoundConstantPushDownExpression,
variableReferencePushDownExpression.get(),
upperBoundConstantPushDownExpression);
if (metadataFilterColumns.contains(variableReferencePushDownExpression.get())) {
metadataSql = format(
"\"%s\" >= %s AND \"%s\" <= %s",
variableReferencePushDownExpression.get(),
lowerBoundConstantPushDownExpression,
variableReferencePushDownExpression.get(),
upperBoundConstantPushDownExpression);
}
return new ClpExpression(kql, metadataSql);
}

@Override
public ClpExpression visitConstant(ConstantExpression node, Void context)
{
Expand Down Expand Up @@ -203,7 +267,13 @@ private ClpExpression handleNot(CallExpression node)
if (expression.getRemainingExpression().isPresent() || !expression.getPushDownExpression().isPresent()) {
return new ClpExpression(node);
}
return new ClpExpression("NOT " + expression.getPushDownExpression().get());
String notPushDownExpression = "NOT " + expression.getPushDownExpression().get();
if (expression.getMetadataSql().isPresent()) {
return new ClpExpression(notPushDownExpression, "NOT " + expression.getMetadataSql());
}
else {
return new ClpExpression(notPushDownExpression);
}
}

/**
Expand Down Expand Up @@ -350,24 +420,40 @@ private ClpExpression buildClpExpression(
Type literalType,
RowExpression originalNode)
{
String metadataSql = null;
if (operator.equals(EQUAL)) {
if (literalType instanceof VarcharType) {
return new ClpExpression(format("%s: \"%s\"", variableName, literalString));
if (metadataFilterColumns.contains(variableName)) {
metadataSql = format("\"%s\" = '%s'", variableName, literalString);
}
return new ClpExpression(format("%s: \"%s\"", variableName, escapeKqlSpecialCharsForStringValue(literalString)), metadataSql);
}
else {
return new ClpExpression(format("%s: %s", variableName, literalString));
if (metadataFilterColumns.contains(variableName)) {
metadataSql = format("\"%s\" = %s", variableName, literalString);
}
return new ClpExpression(format("%s: %s", variableName, literalString), metadataSql);
}
}
else if (operator.equals(NOT_EQUAL)) {
if (literalType instanceof VarcharType) {
return new ClpExpression(format("NOT %s: \"%s\"", variableName, literalString));
if (metadataFilterColumns.contains(variableName)) {
metadataSql = format("NOT \"%s\" = '%s'", variableName, literalString);
}
return new ClpExpression(format("NOT %s: \"%s\"", variableName, escapeKqlSpecialCharsForStringValue(literalString)), metadataSql);
}
else {
return new ClpExpression(format("NOT %s: %s", variableName, literalString));
if (metadataFilterColumns.contains(variableName)) {
metadataSql = format("NOT \"%s\" = %s", variableName, literalString);
}
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Use inequality operator for metadata SQL consistency

For NOT_EQUAL, consider using the inequality operator directly in the metadata SQL instead of NOT "column" = value:

                 if (metadataFilterColumns.contains(variableName)) {
-                    metadataSql = format("NOT \"%s\" = %s", variableName, literalString);
+                    metadataSql = format("\"%s\" != %s", variableName, literalString);
                 }

This makes the SQL more conventional and consistent with the other comparison operators.

📝 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
if (metadataFilterColumns.contains(variableName)) {
metadataSql = format("NOT \"%s\" = %s", variableName, literalString);
}
if (metadataFilterColumns.contains(variableName)) {
metadataSql = format("\"%s\" != %s", variableName, literalString);
}
🤖 Prompt for AI Agents
In
presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpFilterToKqlConverter.java
around lines 440 to 442, replace the use of NOT "column" = value with the direct
inequality operator "column" != value for the metadata SQL when handling
NOT_EQUAL. This change makes the SQL more conventional and consistent with other
comparison operators by using the standard inequality syntax instead of negating
equality.

return new ClpExpression(format("NOT %s: %s", variableName, literalString), metadataSql);
}
}
else if (LOGICAL_BINARY_OPS_FILTER.contains(operator) && !(literalType instanceof VarcharType)) {
return new ClpExpression(format("%s %s %s", variableName, operator.getOperator(), literalString));
if (metadataFilterColumns.contains(variableName)) {
metadataSql = format("\"%s\" %s %s", variableName, operator.getOperator(), literalString);
}
return new ClpExpression(format("%s %s %s", variableName, operator.getOperator(), literalString), metadataSql);
}
return new ClpExpression(originalNode);
}
Expand Down Expand Up @@ -568,16 +654,24 @@ private Optional<Integer> parseLengthLiteral(RowExpression lengthExpression, Str
*/
private ClpExpression handleAnd(SpecialFormExpression node)
{
StringBuilder metadataQueryBuilder = new StringBuilder();
metadataQueryBuilder.append("(");
StringBuilder queryBuilder = new StringBuilder();
queryBuilder.append("(");
List<RowExpression> remainingExpressions = new ArrayList<>();
boolean hasMetadataSql = false;
boolean hasPushDownExpression = false;
for (RowExpression argument : node.getArguments()) {
ClpExpression expression = argument.accept(this, null);
if (expression.getPushDownExpression().isPresent()) {
hasPushDownExpression = true;
queryBuilder.append(expression.getPushDownExpression().get());
queryBuilder.append(" AND ");
if (expression.getMetadataSql().isPresent()) {
hasMetadataSql = true;
metadataQueryBuilder.append(expression.getMetadataSql().get());
metadataQueryBuilder.append(" AND ");
}
}
if (expression.getRemainingExpression().isPresent()) {
remainingExpressions.add(expression.getRemainingExpression().get());
Expand All @@ -588,16 +682,21 @@ private ClpExpression handleAnd(SpecialFormExpression node)
}
else if (!remainingExpressions.isEmpty()) {
if (remainingExpressions.size() == 1) {
return new ClpExpression(queryBuilder.substring(0, queryBuilder.length() - 5) + ")", remainingExpressions.get(0));
return new ClpExpression(
queryBuilder.substring(0, queryBuilder.length() - 5) + ")",
hasMetadataSql ? metadataQueryBuilder.substring(0, metadataQueryBuilder.length() - 5) + ")" : null,
remainingExpressions.get(0));
}
else {
return new ClpExpression(
queryBuilder.substring(0, queryBuilder.length() - 5) + ")",
hasMetadataSql ? metadataQueryBuilder.substring(0, metadataQueryBuilder.length() - 5) + ")" : null,
new SpecialFormExpression(node.getSourceLocation(), AND, BOOLEAN, remainingExpressions));
}
}
// Remove the last " AND " from the query
return new ClpExpression(queryBuilder.substring(0, queryBuilder.length() - 5) + ")");
return new ClpExpression(queryBuilder.substring(0, queryBuilder.length() - 5) + ")",
hasMetadataSql ? metadataQueryBuilder.substring(0, metadataQueryBuilder.length() - 5) + ")" : null);
}

/**
Expand All @@ -614,18 +713,30 @@ else if (!remainingExpressions.isEmpty()) {
*/
private ClpExpression handleOr(SpecialFormExpression node)
{
StringBuilder metadataQueryBuilder = new StringBuilder();
metadataQueryBuilder.append("(");
StringBuilder queryBuilder = new StringBuilder();
queryBuilder.append("(");
boolean hasAllMetadataSql = true;
for (RowExpression argument : node.getArguments()) {
ClpExpression expression = argument.accept(this, null);
if (expression.getRemainingExpression().isPresent() || !expression.getPushDownExpression().isPresent()) {
return new ClpExpression(node);
}
queryBuilder.append(expression.getPushDownExpression().get());
queryBuilder.append(" OR ");
if (hasAllMetadataSql && expression.getMetadataSql().isPresent()) {
metadataQueryBuilder.append(expression.getMetadataSql().get());
metadataQueryBuilder.append(" OR ");
}
else {
hasAllMetadataSql = false;
}
}
// Remove the last " OR " from the query
return new ClpExpression(queryBuilder.substring(0, queryBuilder.length() - 4) + ")");
return new ClpExpression(
queryBuilder.substring(0, queryBuilder.length() - 4) + ")",
hasAllMetadataSql ? metadataQueryBuilder.substring(0, metadataQueryBuilder.length() - 4) + ")" : null);
}

/**
Expand Down
Loading
Loading