Skip to content
Merged
Show file tree
Hide file tree
Changes from 17 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 @@ -18,22 +18,30 @@
import java.util.Optional;

/**
* Represents the result of converting a Presto RowExpression into a CLP-compatible KQL query. In
* every case, `pushDownExpression` represents the part of the RowExpression that could be
* converted to a KQL expression, and `remainingExpression` represents the part that could not be
* converted.
* Represents the result of:
* <ul>
* <li>splitting a {@link RowExpression} into an expression that can be pushed down to CLP
* (`pushDownExpression`) and any remaining expression that cannot be
* (`remainingExpression`).</li>
* <li>a SQL query for filtering splits using CLP's metadata database.</li>
* </ul>
*/
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 query plan, which is only made of up of columns in
// CLP's metadata database.
private final Optional<String> metadataSqlQuery;

// 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 metadataSqlQuery, RowExpression remainingExpression)
{
this.pushDownExpression = Optional.ofNullable(pushDownExpression);
this.metadataSqlQuery = Optional.ofNullable(metadataSqlQuery);
this.remainingExpression = Optional.ofNullable(remainingExpression);
}

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

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

/**
* Creates a ClpExpression from a fully translatable KQL string or column name, as well as a
* metadata SQL string.
*
* @param pushDownExpression
* @param metadataSqlQuery
*/
public ClpExpression(String pushDownExpression, String metadataSqlQuery)
{
this(pushDownExpression, metadataSqlQuery, null);
}

/**
Expand All @@ -62,14 +82,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> getMetadataSqlQuery()
{
return metadataSqlQuery;
}

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