Skip to content

refactor: Improve metadata expression extraction and validation; Update split metadata config format; Add split metadata columns during metadata resolution. #89

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

Open
wants to merge 27 commits into
base: release-0.293-clp-connector
Choose a base branch
from
Open

refactor: Improve metadata expression extraction and validation; Update split metadata config format; Add split metadata columns during metadata resolution. #89

Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
68dde54
refactor progress
wraymo Oct 27, 2025
aa85805
progress
wraymo Nov 3, 2025
7164767
finish refactoring metadata expression generation
wraymo Nov 4, 2025
8e077fc
progress
wraymo Nov 4, 2025
b191322
add converter
wraymo Nov 6, 2025
9cd2d4b
refactor tests
wraymo Nov 6, 2025
999a6fa
format fix
wraymo Nov 7, 2025
422ff69
doc update
wraymo Nov 7, 2025
e517b17
doc update
wraymo Nov 7, 2025
01aefab
refactor
wraymo Nov 7, 2025
cdebd8d
fix compilation errors
wraymo Nov 7, 2025
6430711
fix issues in TestClpSplitMetadataConfigCommon
wraymo Nov 7, 2025
c5b6776
fix TestClpUdfRewriter
wraymo Nov 7, 2025
d816d79
fix TestClpMySqlSplitMetadataConfig and TestClpSplit
wraymo Nov 7, 2025
e8e8334
clean up the docs
wraymo Nov 7, 2025
9b54813
fix bugs
wraymo Nov 7, 2025
f08107d
add comments
wraymo Nov 7, 2025
11cf752
minor work
wraymo Nov 7, 2025
86f629c
doc improvement
wraymo Nov 7, 2025
37e3235
Merge branch 'release-0.293-clp-connector' into metadata_refactor
wraymo Nov 7, 2025
7a283a7
rename
wraymo Nov 7, 2025
a6b71f6
add more test cases and fix bugs
wraymo Nov 8, 2025
d988cbc
apply coderabbit suggestion
wraymo Nov 8, 2025
fbc6498
Added support for incorporating metadata columns during metadata reso…
wraymo Nov 10, 2025
f2db72c
apply coderabbit suggestions
wraymo Nov 10, 2025
5e58a60
fix format issue
wraymo Nov 11, 2025
8137399
validate required columns in split provider
wraymo Nov 11, 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
5 changes: 0 additions & 5 deletions presto-clp/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,11 +65,6 @@
<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>
Expand Down
30 changes: 6 additions & 24 deletions presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,7 @@ public class ClpConfig
private long metadataRefreshInterval = 60;
private long metadataExpireInterval = 600;

private String splitFilterConfig;
private SplitFilterProviderType splitFilterProviderType = SplitFilterProviderType.MYSQL;
private String splitMetadataConfigPath;
private SplitProviderType splitProviderType = SplitProviderType.MYSQL;

public boolean isPolymorphicTypeEnabled()
Expand Down Expand Up @@ -151,27 +150,15 @@ public ClpConfig setMetadataExpireInterval(long metadataExpireInterval)
return this;
}

public String getSplitFilterConfig()
public String getSplitMetadataConfigPath()
{
return splitFilterConfig;
return splitMetadataConfigPath;
}

@Config("clp.split-filter-config")
public ClpConfig setSplitFilterConfig(String splitFilterConfig)
@Config("clp.split-metadata-config-path")
public ClpConfig setSplitMetadataConfigPath(String splitMetadataConfigPath)
{
this.splitFilterConfig = splitFilterConfig;
return this;
}

public SplitFilterProviderType getSplitFilterProviderType()
{
return splitFilterProviderType;
}

@Config("clp.split-filter-provider-type")
public ClpConfig setSplitFilterProviderType(SplitFilterProviderType splitFilterProviderType)
{
this.splitFilterProviderType = splitFilterProviderType;
this.splitMetadataConfigPath = splitMetadataConfigPath;
return this;
}

Expand All @@ -192,11 +179,6 @@ public enum MetadataProviderType
MYSQL
}

public enum SplitFilterProviderType
{
MYSQL
}

public enum SplitProviderType
{
MYSQL
Expand Down
10 changes: 5 additions & 5 deletions presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpConnector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
import com.facebook.airlift.bootstrap.LifeCycleManager;
import com.facebook.airlift.log.Logger;
import com.facebook.presto.plugin.clp.optimization.ClpPlanOptimizerProvider;
import com.facebook.presto.plugin.clp.split.filter.ClpSplitFilterProvider;
import com.facebook.presto.plugin.clp.split.ClpSplitMetadataConfig;
import com.facebook.presto.spi.connector.Connector;
import com.facebook.presto.spi.connector.ConnectorMetadata;
import com.facebook.presto.spi.connector.ConnectorPlanOptimizerProvider;
Expand All @@ -42,7 +42,7 @@ public class ClpConnector
private final ClpSplitManager splitManager;
private final FunctionMetadataManager functionManager;
private final StandardFunctionResolution functionResolution;
private final ClpSplitFilterProvider splitFilterProvider;
private final ClpSplitMetadataConfig clpSplitMetadataConfig;

@Inject
public ClpConnector(
Expand All @@ -52,21 +52,21 @@ public ClpConnector(
ClpSplitManager splitManager,
FunctionMetadataManager functionManager,
StandardFunctionResolution functionResolution,
ClpSplitFilterProvider splitFilterProvider)
ClpSplitMetadataConfig clpSplitMetadataConfig)
{
this.lifeCycleManager = requireNonNull(lifeCycleManager, "lifeCycleManager is null");
this.metadata = requireNonNull(metadata, "metadata is null");
this.recordSetProvider = requireNonNull(recordSetProvider, "recordSetProvider is null");
this.splitManager = requireNonNull(splitManager, "splitManager is null");
this.functionManager = requireNonNull(functionManager, "functionManager is null");
this.functionResolution = requireNonNull(functionResolution, "functionResolution is null");
this.splitFilterProvider = requireNonNull(splitFilterProvider, "splitFilterProvider is null");
this.clpSplitMetadataConfig = requireNonNull(clpSplitMetadataConfig);
}

@Override
public ConnectorPlanOptimizerProvider getConnectorPlanOptimizerProvider()
{
return new ClpPlanOptimizerProvider(functionManager, functionResolution, splitFilterProvider);
return new ClpPlanOptimizerProvider(functionManager, functionResolution, clpSplitMetadataConfig);
}

@Override
Expand Down
1 change: 1 addition & 0 deletions presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpConnectorFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ public Connector create(String catalogName, Map<String, String> config, Connecto
try {
Bootstrap app = new Bootstrap(new JsonModule(), new ClpModule(), binder -> {
binder.bind(FunctionMetadataManager.class).toInstance(context.getFunctionMetadataManager());
binder.bind(TypeManager.class).toInstance(context.getTypeManager());
binder.bind(NodeManager.class).toInstance(context.getNodeManager());
binder.bind(StandardFunctionResolution.class).toInstance(context.getStandardFunctionResolution());
binder.bind(TypeManager.class).toInstance(context.getTypeManager());
Expand Down
5 changes: 2 additions & 3 deletions presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpErrorCode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,8 @@ public enum ClpErrorCode
CLP_UNSUPPORTED_TYPE(3, EXTERNAL),
CLP_UNSUPPORTED_CONFIG_OPTION(4, EXTERNAL),

CLP_SPLIT_FILTER_CONFIG_NOT_FOUND(10, USER_ERROR),
CLP_MANDATORY_SPLIT_FILTER_NOT_VALID(11, USER_ERROR),
CLP_UNSUPPORTED_SPLIT_FILTER_SOURCE(12, EXTERNAL);
CLP_SPLIT_METADATA_CONFIG_NOT_FOUND(10, USER_ERROR),
CLP_MANDATORY_COLUMN_NOT_IN_FILTER(11, USER_ERROR);

private final ErrorCode errorCode;

Expand Down
13 changes: 2 additions & 11 deletions presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,19 +17,16 @@
import com.facebook.presto.plugin.clp.metadata.ClpMetadataProvider;
import com.facebook.presto.plugin.clp.metadata.ClpMySqlMetadataProvider;
import com.facebook.presto.plugin.clp.split.ClpMySqlSplitProvider;
import com.facebook.presto.plugin.clp.split.ClpSplitMetadataConfig;
import com.facebook.presto.plugin.clp.split.ClpSplitProvider;
import com.facebook.presto.plugin.clp.split.filter.ClpMySqlSplitFilterProvider;
import com.facebook.presto.plugin.clp.split.filter.ClpSplitFilterProvider;
import com.facebook.presto.spi.PrestoException;
import com.google.inject.Binder;
import com.google.inject.Scopes;

import static com.facebook.airlift.configuration.ConfigBinder.configBinder;
import static com.facebook.presto.plugin.clp.ClpConfig.MetadataProviderType;
import static com.facebook.presto.plugin.clp.ClpConfig.SplitFilterProviderType;
import static com.facebook.presto.plugin.clp.ClpConfig.SplitProviderType;
import static com.facebook.presto.plugin.clp.ClpErrorCode.CLP_UNSUPPORTED_METADATA_SOURCE;
import static com.facebook.presto.plugin.clp.ClpErrorCode.CLP_UNSUPPORTED_SPLIT_FILTER_SOURCE;
import static com.facebook.presto.plugin.clp.ClpErrorCode.CLP_UNSUPPORTED_SPLIT_SOURCE;

public class ClpModule
Expand All @@ -42,17 +39,11 @@ protected void setup(Binder binder)
binder.bind(ClpMetadata.class).in(Scopes.SINGLETON);
binder.bind(ClpRecordSetProvider.class).in(Scopes.SINGLETON);
binder.bind(ClpSplitManager.class).in(Scopes.SINGLETON);
binder.bind(ClpSplitMetadataConfig.class).in(Scopes.SINGLETON);
configBinder(binder).bindConfig(ClpConfig.class);

ClpConfig config = buildConfigObject(ClpConfig.class);

if (SplitFilterProviderType.MYSQL == config.getSplitFilterProviderType()) {
binder.bind(ClpSplitFilterProvider.class).to(ClpMySqlSplitFilterProvider.class).in(Scopes.SINGLETON);
}
else {
throw new PrestoException(CLP_UNSUPPORTED_SPLIT_FILTER_SOURCE, "Unsupported split filter provider type: " + config.getSplitFilterProviderType());
}

if (config.getMetadataProviderType() == MetadataProviderType.MYSQL) {
binder.bind(ClpMetadataProvider.class).to(ClpMySqlMetadataProvider.class).in(Scopes.SINGLETON);
}
Expand Down
20 changes: 12 additions & 8 deletions presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpTableLayoutHandle.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
package com.facebook.presto.plugin.clp;

import com.facebook.presto.spi.ConnectorTableLayoutHandle;
import com.facebook.presto.spi.relation.RowExpression;
import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonProperty;

Expand All @@ -27,14 +28,17 @@ public class ClpTableLayoutHandle
{
private final ClpTableHandle table;
private final Optional<String> kqlQuery;
private final Optional<String> metadataSql;
private final Optional<RowExpression> metadataExpression;

@JsonCreator
public ClpTableLayoutHandle(@JsonProperty("table") ClpTableHandle table, @JsonProperty("kqlQuery") Optional<String> kqlQuery, @JsonProperty("metadataFilterQuery") Optional<String> metadataSql)
public ClpTableLayoutHandle(
@JsonProperty("table") ClpTableHandle table,
@JsonProperty("kqlQuery") Optional<String> kqlQuery,
@JsonProperty("metadataExpression") Optional<RowExpression> metadataSqlExpression)
{
this.table = table;
this.kqlQuery = kqlQuery;
this.metadataSql = metadataSql;
this.metadataExpression = metadataSqlExpression;
}

@JsonProperty
Expand All @@ -50,9 +54,9 @@ public Optional<String> getKqlQuery()
}

@JsonProperty
public Optional<String> getMetadataSql()
public Optional<RowExpression> getMetadataExpression()
{
return metadataSql;
return metadataExpression;
}

@Override
Expand All @@ -67,13 +71,13 @@ public boolean equals(Object o)
ClpTableLayoutHandle that = (ClpTableLayoutHandle) o;
return Objects.equals(table, that.table) &&
Objects.equals(kqlQuery, that.kqlQuery) &&
Objects.equals(metadataSql, that.metadataSql);
Objects.equals(metadataExpression, that.metadataExpression);
}

@Override
public int hashCode()
{
return Objects.hash(table, kqlQuery, metadataSql);
return Objects.hash(table, kqlQuery, metadataExpression);
}

@Override
Expand All @@ -82,7 +86,7 @@ public String toString()
return toStringHelper(this)
.add("table", table)
.add("kqlQuery", kqlQuery)
.add("metadataSql", metadataSql)
.add("metadataExpression", metadataExpression)
.toString();
}
}
60 changes: 21 additions & 39 deletions presto-clp/src/main/java/com/facebook/presto/plugin/clp/optimization/ClpComputePushDown.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,15 @@
package com.facebook.presto.plugin.clp.optimization;

import com.facebook.airlift.log.Logger;
import com.facebook.presto.plugin.clp.ClpExpression;
import com.facebook.presto.plugin.clp.ClpTableHandle;
import com.facebook.presto.plugin.clp.ClpTableLayoutHandle;
import com.facebook.presto.plugin.clp.split.filter.ClpSplitFilterProvider;
import com.facebook.presto.plugin.clp.split.ClpSplitMetadataConfig;
import com.facebook.presto.spi.ColumnHandle;
import com.facebook.presto.spi.ConnectorPlanOptimizer;
import com.facebook.presto.spi.ConnectorPlanRewriter;
import com.facebook.presto.spi.ConnectorSession;
import com.facebook.presto.spi.PrestoException;
import com.facebook.presto.spi.SchemaTableName;
import com.facebook.presto.spi.TableHandle;
import com.facebook.presto.spi.VariableAllocator;
import com.facebook.presto.spi.function.FunctionMetadataManager;
Expand All @@ -32,16 +33,12 @@
import com.facebook.presto.spi.plan.TableScanNode;
import com.facebook.presto.spi.relation.RowExpression;
import com.facebook.presto.spi.relation.VariableReferenceExpression;
import com.google.common.collect.ImmutableSet;

import java.util.HashSet;
import java.util.Map;
import java.util.Optional;
import java.util.Set;

import static com.facebook.presto.plugin.clp.ClpConnectorFactory.CONNECTOR_NAME;
import static com.facebook.presto.plugin.clp.ClpErrorCode.CLP_MANDATORY_COLUMN_NOT_IN_FILTER;
import static com.facebook.presto.spi.ConnectorPlanRewriter.rewriteWith;
import static java.lang.String.format;
import static java.util.Objects.requireNonNull;

public class ClpComputePushDown
Expand All @@ -50,48 +47,44 @@ public class ClpComputePushDown
private static final Logger log = Logger.get(ClpComputePushDown.class);
private final FunctionMetadataManager functionManager;
private final StandardFunctionResolution functionResolution;
private final ClpSplitFilterProvider splitFilterProvider;
private final ClpSplitMetadataConfig metadataConfig;

public ClpComputePushDown(FunctionMetadataManager functionManager, StandardFunctionResolution functionResolution, ClpSplitFilterProvider splitFilterProvider)
public ClpComputePushDown(
FunctionMetadataManager functionManager,
StandardFunctionResolution functionResolution,
ClpSplitMetadataConfig metadataConfig)
{
this.functionManager = requireNonNull(functionManager, "functionManager is null");
this.functionResolution = requireNonNull(functionResolution, "functionResolution is null");
this.splitFilterProvider = requireNonNull(splitFilterProvider, "splitFilterProvider is null");
this.metadataConfig = requireNonNull(metadataConfig, "metadataConfig is null");
}

@Override
public PlanNode optimize(PlanNode maxSubplan, ConnectorSession session, VariableAllocator variableAllocator, PlanNodeIdAllocator idAllocator)
{
Rewriter rewriter = new Rewriter(idAllocator);
PlanNode optimizedPlanNode = rewriteWith(rewriter, maxSubplan);

// Throw exception if any required split filters are missing
if (!rewriter.tableScopeSet.isEmpty() && !rewriter.hasVisitedFilter) {
splitFilterProvider.checkContainsRequiredFilters(rewriter.tableScopeSet, "");
}
return optimizedPlanNode;
return rewriteWith(rewriter, maxSubplan);
}

private class Rewriter
extends ConnectorPlanRewriter<Void>
{
private final PlanNodeIdAllocator idAllocator;
private final Set<String> tableScopeSet;
private boolean hasVisitedFilter;

public Rewriter(PlanNodeIdAllocator idAllocator)
{
this.idAllocator = idAllocator;
hasVisitedFilter = false;
tableScopeSet = new HashSet<>();
}

@Override
public PlanNode visitTableScan(TableScanNode node, RewriteContext<Void> context)
{
TableHandle tableHandle = node.getTable();
ClpTableHandle clpTableHandle = (ClpTableHandle) tableHandle.getConnectorHandle();
tableScopeSet.add(format("%s.%s", CONNECTOR_NAME, clpTableHandle.getSchemaTableName()));
if (!metadataConfig.getRequiredColumns(clpTableHandle.getSchemaTableName()).isEmpty()) {
throw new PrestoException(CLP_MANDATORY_COLUMN_NOT_IN_FILTER, "required filters must be specified");
}
Copy link

@coderabbitai coderabbitai bot Nov 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Don’t throw before metadata filters are processed.

visitTableScan is invoked while rewriting every table scan, including ones that still have a metadata filter sitting above them. Because the new guard executes before the filter is analysed, any table that lists required columns now fails even when the query provides the correct metadata filter. We need to defer the “required filters must be specified” check until after we’ve inspected the filter’s RowExpression (so genuine missing filters still fail, but valid ones pass).

Apply this diff to remove the premature throw:

-            if (!metadataConfig.getRequiredColumns(clpTableHandle.getSchemaTableName()).isEmpty()) {
-                throw new PrestoException(CLP_MANDATORY_COLUMN_NOT_IN_FILTER, "required filters must be specified");
-            }

Then, inside processFilter, enforce the requirement only when no metadata expression was produced:

@@
-            SchemaTableName schemaTableName = clpTableHandle.getSchemaTableName();
+            SchemaTableName schemaTableName = clpTableHandle.getSchemaTableName();
+            Set<String> requiredColumns = metadataConfig.getRequiredColumns(schemaTableName);
@@
-            Optional<RowExpression> metadataExpression = clpExpression.getMetadataExpression();
+            Optional<RowExpression> metadataExpression = clpExpression.getMetadataExpression();
+
+            if (!requiredColumns.isEmpty() && !metadataExpression.isPresent()) {
+                throw new PrestoException(CLP_MANDATORY_COLUMN_NOT_IN_FILTER, "required filters must be specified");
+            }

Don’t forget to add import java.util.Set; at the top. This keeps the enforcement intact for missing filters while allowing valid filtered queries to proceed.

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents 
In
presto-clp/src/main/java/com/facebook/presto/plugin/clp/optimization/ClpComputePushDown.java
around lines 82-86, remove the premature PrestoException throw that checks
metadataConfig.getRequiredColumns(...) in visitTableScan so the check does not
run before the metadata filter is processed; then update processFilter to
enforce the "required filters must be specified" rule only when no metadata
expression was produced (i.e. when the metadata expression is absent/empty), and
add an import line for java.util.Set at the top of the file.

Copy link
Author

@wraymo wraymo Nov 8, 2025

Choose a reason for hiding this comment

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

There could be no filters. No filter should be treated as a failure if required columns are specified

Copy link
Author

@wraymo wraymo Nov 8, 2025

Choose a reason for hiding this comment

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

That's why we need to do it in visitTableScan

Copy link

@coderabbitai coderabbitai bot Nov 8, 2025

Choose a reason for hiding this comment

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

@wraymo, you're absolutely right! The check in visitTableScan is necessary and correct for the no-filter case.

The visitor pattern here works as follows:

  • When there's no FilterNode above the TableScanNode: visitTableScan is called, and the required-columns check appropriately throws an exception.
  • When there IS a FilterNode above the TableScanNode: visitFilter is called, which directly invokes processFilter on the source TableScanNode without delegating to visitTableScan. This means visitTableScan is never invoked in this path.

So the check in visitTableScan only fires when there's no filter, which is exactly when you want to reject queries that don't provide the required metadata filters. It doesn't interfere with queries that have filters because those go through the visitFilter → processFilter path instead.

CodeRabbit's concern about a "premature throw before metadata filters are processed" doesn't apply here—when a metadata filter exists in the plan, visitTableScan isn't called at all.

One minor observation (not blocking): processFilter could also add a check to ensure that when required columns exist and the filter produces no metadataExpression, an exception is thrown. This would catch the edge case where a filter exists but doesn't cover the required metadata columns. However, that might be a separate consideration beyond the scope of this specific discussion.

🧠 Learnings used 
Learnt from: anlowee
Repo: y-scope/presto PR: 46
File: presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpPlanOptimizer.java:105-105
Timestamp: 2025-07-30T15:27:04.862Z
Learning: In ClpPlanOptimizer, the local construction of tableScope in visitFilter() method is necessary and correct because join queries can have multiple TableScanNodes for different tables. Each FilterNode needs to determine its scope from its immediate source TableScanNode, not from a shared instance field that gets overwritten.

Learnt from: anlowee
Repo: y-scope/presto PR: 64
File: presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpFilterToKql.java:284-299
Timestamp: 2025-09-16T17:39:45.102Z
Learning: CLP wildcard UDFs (CLP_WILDCARD_STRING_COLUMN, CLP_WILDCARD_INT_COLUMN, etc.) cannot be used as split filters (formerly called metadata filters) because the split filter mechanism checks whether the filter contains field names that are defined in the split filter configuration file. Since wildcard functions don't correspond to specific column names, they don't pass this validation.

Copy link

@coderabbitai coderabbitai bot Nov 8, 2025

Choose a reason for hiding this comment

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

@wraymo, you're absolutely right! I see the distinction now.

The visitor flow means:

  • Query with no filter → visitTableScan is called → your check catches it ✓
  • Query with filter → visitFilter is called → processFilter handles it directly → visitTableScan is NOT called for that TableScanNode

So the check in visitTableScan specifically guards against queries that have zero filters when required columns are configured. That's the correct placement for that scenario.

One follow-up question: for queries that do have a filter but the filter doesn't reference the required columns (doesn't produce a metadataExpression), where is that validated? I see that processFilter extracts the metadataExpression but doesn't appear to check whether required columns were satisfied. Is that validation happening later in ClpMySqlSplitMetadataExpressionConverter.transform() during split generation, or should we add an explicit check in processFilter to fail fast at plan time?

🧠 Learnings used 
Learnt from: anlowee
Repo: y-scope/presto PR: 46
File: presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpPlanOptimizer.java:105-105
Timestamp: 2025-07-30T15:27:04.862Z
Learning: In ClpPlanOptimizer, the local construction of tableScope in visitFilter() method is necessary and correct because join queries can have multiple TableScanNodes for different tables. Each FilterNode needs to determine its scope from its immediate source TableScanNode, not from a shared instance field that gets overwritten.

Learnt from: anlowee
Repo: y-scope/presto PR: 64
File: presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpFilterToKql.java:284-299
Timestamp: 2025-09-16T17:39:45.102Z
Learning: CLP wildcard UDFs (CLP_WILDCARD_STRING_COLUMN, CLP_WILDCARD_INT_COLUMN, etc.) cannot be used as split filters (formerly called metadata filters) because the split filter mechanism checks whether the filter contains field names that are defined in the split filter configuration file. Since wildcard functions don't correspond to specific column names, they don't pass this validation.


return super.visitTableScan(node, context);
}

Expand All @@ -107,40 +100,29 @@ public PlanNode visitFilter(FilterNode node, RewriteContext<Void> context)

private PlanNode processFilter(FilterNode filterNode, TableScanNode tableScanNode)
{
hasVisitedFilter = true;

TableHandle tableHandle = tableScanNode.getTable();
ClpTableHandle clpTableHandle = (ClpTableHandle) tableHandle.getConnectorHandle();

String tableScope = CONNECTOR_NAME + "." + clpTableHandle.getSchemaTableName().toString();
Map<VariableReferenceExpression, ColumnHandle> assignments = tableScanNode.getAssignments();

SchemaTableName schemaTableName = clpTableHandle.getSchemaTableName();
ClpExpression clpExpression = filterNode.getPredicate().accept(
new ClpFilterToKqlConverter(
functionResolution,
functionManager,
assignments,
splitFilterProvider.getColumnNames(tableScope)),
metadataConfig.getMetadataColumns(schemaTableName).keySet(),
metadataConfig.getDataColumnsWithRangeBounds(schemaTableName)),
null);
Optional<String> kqlQuery = clpExpression.getPushDownExpression();
Optional<String> metadataSqlQuery = clpExpression.getMetadataSqlQuery();
Optional<RowExpression> metadataExpression = clpExpression.getMetadataExpression();
Optional<RowExpression> remainingPredicate = clpExpression.getRemainingExpression();

// Perform required metadata filter checks before handling the KQL query (if kqlQuery
// isn't present, we'll return early, skipping subsequent checks).
splitFilterProvider.checkContainsRequiredFilters(ImmutableSet.of(tableScope), metadataSqlQuery.orElse(""));
boolean hasMetadataFilter = metadataSqlQuery.isPresent() && !metadataSqlQuery.get().isEmpty();
if (hasMetadataFilter) {
metadataSqlQuery = Optional.of(splitFilterProvider.remapSplitFilterPushDownExpression(tableScope, metadataSqlQuery.get()));
log.debug("Metadata SQL query: %s", metadataSqlQuery.get());
}

if (kqlQuery.isPresent() || hasMetadataFilter) {
if (kqlQuery.isPresent() || metadataExpression.isPresent()) {
if (kqlQuery.isPresent()) {
log.debug("KQL query: %s", kqlQuery.get());
}

ClpTableLayoutHandle layoutHandle = new ClpTableLayoutHandle(clpTableHandle, kqlQuery, metadataSqlQuery);
ClpTableLayoutHandle layoutHandle = new ClpTableLayoutHandle(clpTableHandle, kqlQuery, metadataExpression);
TableHandle newTableHandle = new TableHandle(
tableHandle.getConnectorId(),
clpTableHandle,
Expand Down
Loading
Loading