refactor: Extract split filter provider interface to improve flexibility for user customization. #46

anlowee · 2025-07-21T16:56:33Z

Description

In this PR we mainly extract an interface of a split filter provider, so that the user can extend their own config format for their own metadata database. Since the metadata filter is only used for split filtering, we decide to rename it to split filter.

We can add a new package: split.filter, and a base class ClpSplitFilterProvider.

In this base class we keep all functions for processing scope, which is a concept that in the previous metadata filter design (i.e., is the filter for all schemas and all tables, or for tables under a certain schema, or only for the certain table). And also two abstract functions:

getCustomSplitFilterOptionsClass, which is for the user to return their own implementation for CustomSplitFilterOptions class. This is also a guard so that the user won’t forget to implement the CustomSplitFilterOptions class (see below for the definition of CustomSplitFilterOptions).
remapSplitFilterPushDown, which is renamed from the old function remapFilterSql. This function is for rewriting the split filter push down expression with split-filter-specific stuff, so the user can only extend this function to handle split-filter-specific logic.

For the current filter structure, we move it to a separate class ClpMetadataFilter which defined the basic structure for filters of all types of metadata database:

”columnName”: the filter name.
”customOptions”: the split-filter-specific data structure which should be implemented by the user, where we can put all split-filter-specific fields in it (e.g., ”rangeMapping”). We provide an implementation for MySql metadata database in ClpMySqlSplitFilterProvider.SplitDatabaseSpecific.
”required”: does the filter have to exist after push down.
As discussed, we will move this field into ”customOptions” in the next PR.

We also add a new class ClpCustomSplitFilterOptionsDeserializer which is for deserializing the ”customOptions” field with the class given by getCustomSplitFilterOptionsClass.

For the changes in the config file for MySql metadata database, we only move the “rangeMapping” into the field ”customOptions”:

{
  "clp.default.table_1": [
      {
        "columnName": "msg.timestamp",
        "customOptions": {
          "rangeMapping": {
            "lowerBound": "begin_timestamp",
            "upperBound": "end_timestamp"
          }
        },
        "required": true
      },
      {
        "columnName": "file_name"
      }
  ]
}

Checklist

The PR satisfies the contribution guidelines.
This is a breaking change and that has been indicated in the PR title, OR this isn't a
breaking change.
Necessary docs have been updated, OR no docs need to be updated.

Validation performed

E2E test w/ w/o metadata filters. Using pregsql datasets and configuring the timestamp as a required metadata filter:

SELECT 1 = 1 FROM default LIMIT 1, throw exception due to missing, because even it is not querying on any columns it still scans the table.
SELECT query_id + 1 FROM default LIMIT 1, throw exception due to missing.
SELECT * FROM default WHERE ps = 'startup' LIMIT 1, throw exception due to missing.
SELECT COUNT(*) FROM default WHERE timestamp > FROM_UNIXTIME(0) AND timestamp < FROM_UNIXTIME(9999999999.1234);, return correctly.

Passed CI.

Summary by CodeRabbit

New Features
- Introduces Split Filter support with a configurable provider (MySQL).
- New settings: clp.split-filter-config and clp.split-filter-provider-type.
- Enhanced split-pruning via range-mapping for numeric filters; validates required filters.
Refactor
- Terminology shift from “Metadata Filter” to “Split Filter”.
- Old clp.metadata-filter-config removed; update configurations accordingly.
- Error identifiers updated to Split Filter terminology; adds unsupported provider error.
Documentation
- Overhauled connector docs: new Split Filter configuration, examples, and provider guidance.
- Expanded type mapping, SQL support notes, and revised configuration walkthroughs.

coderabbitai · 2025-07-21T16:56:39Z

Walkthrough

Replaces the legacy metadata-filter mechanism with a new split-filter framework. Introduces ClpSplitFilterProvider (with a MySQL implementation), updates wiring (connector, module, optimizer, provider), adjusts config and error codes, deletes ClpMetadataFilterProvider, updates docs, and adds tests and a JSON resource for the new split-filter configuration.

Changes

Cohort / File(s)	Summary of changes
Wiring (Connector, Module, OptimizerProvider) `presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpConnector.java`, `.../clp/ClpModule.java`, `.../clp/ClpPlanOptimizerProvider.java`	Replace ClpMetadataFilterProvider with ClpSplitFilterProvider across fields, constructors, and usage. ClpModule binds ClpSplitFilterProvider to ClpMySqlSplitFilterProvider based on ClpConfig.SplitFilterProviderType; throws on unsupported types.
Plan Optimizer `presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpPlanOptimizer.java`	Switch to ClpSplitFilterProvider. Track table scopes in rewriter, call checkContainsRequiredFilters when applicable, and remap pushdown via remapSplitFilterPushDownExpression. Imports and variable names updated.
Split Filter Framework (New) `.../clp/split/filter/ClpSplitFilterProvider.java`, `.../clp/split/filter/ClpSplitFilterConfig.java`, `.../clp/split/filter/ClpSplitFilterConfigCustomOptionsDeserializer.java`, `.../clp/split/filter/ClpMySqlSplitFilterProvider.java`	Add framework to load per-scope split-filter JSON, expose getColumnNames, required-filter checks, and provider-specific pushdown remapping. Provide MySQL-specific implementation and customOptions deserializer.
Config & Error Codes `presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpConfig.java`, `.../clp/ClpErrorCode.java`	Replace metadata-filter config with split-filter config and provider type. Add SplitFilterProviderType enum. Rename error codes to split-filter variants and add CLP_UNSUPPORTED_SPLIT_FILTER_SOURCE.
Legacy Removal `presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpMetadataFilterProvider.java`	Delete legacy metadata filter provider class.
Documentation `presto-docs/src/main/sphinx/connector/clp.rst`	Update docs to split-filter terminology, configuration keys, provider model, customOptions, examples, and SQL mapping notes.
Tests & Resources `presto-clp/src/test/java/com/facebook/presto/plugin/clp/split/filter/TestClpSplitFilterConfigCommon.java`, `.../split/filter/TestClpMySqlSplitFilterConfig.java`, `presto-clp/src/test/resources/test-mysql-split-filter.json`	Add MySQL remapping tests and common split-filter tests; migrate from metadata-filter tests. Update test JSON to nest rangeMapping under customOptions.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor User
  participant Planner as Presto Planner
  participant Optimizer as ClpPlanOptimizer
  participant Rewriter as Plan Rewriter
  participant Provider as ClpSplitFilterProvider
  Note over Planner,Optimizer: Physical planning phase

  User->>Planner: Submit query
  Planner->>Optimizer: optimize(plan)
  Optimizer->>Rewriter: rewrite(plan)
  Rewriter->>Rewriter: visitTableScan -> collect tableScope
  alt Filter on TableScan
    Rewriter->>Provider: getColumnNames(tableScope)
    Rewriter->>Provider: remapSplitFilterPushDownExpression(tableScope, pushdownExpr)
    Rewriter-->>Optimizer: updated Filter/TableScan nodes
  else No filter visited but tables seen
    Optimizer->>Provider: checkContainsRequiredFilters({tableScopes}, "")
    Provider-->>Optimizer: ok or throws
  end
  Optimizer-->>Planner: optimized plan

sequenceDiagram
  autonumber
  participant Guice as Guice Binder
  participant Config as ClpConfig
  participant Module as ClpModule
  participant Provider as ClpSplitFilterProvider
  Note over Module,Provider: Startup binding

  Guice->>Config: read clp.split-filter-provider-type
  alt type == MYSQL
    Guice->>Module: bind(ClpSplitFilterProvider -> ClpMySqlSplitFilterProvider)
    Guice->>Provider: inject with clp.split-filter-config
  else unsupported
    Module->>Guice: throw CLP_UNSUPPORTED_SPLIT_FILTER_SOURCE
  end

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

feat: Add support for defining and using specific columns for metadata filtering. #19 — Earlier wiring changes in CLP provider/optimizer; this PR continues by refactoring to the split-filter provider.
fix: Register CLP plan optimizer (fixes #35). #33 — Adjusts ClpConnector optimizer wiring; directly related to replacing the injected provider with ClpSplitFilterProvider.
feat: Make metadata filter config optional (resolves #29). #32 — Modifies the legacy ClpMetadataFilterProvider config handling; this PR supersedes it by removing/replacing that provider.

Tip

🔌 Remote MCP (Model Context Protocol) integration is now available!

Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats.

✨ Finishing Touches

📝 Generate Docstrings

🧪 Generate unit tests

Create PR with unit tests
Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>, please review it.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
Add @coderabbitai anywhere in the PR title to generate the title automatically.

Status, Documentation and Community

Visit our Status Page to check the current availability of CodeRabbit.
Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

anlowee · 2025-07-21T18:13:33Z

Note that this PR exceeds the 400 lines is because I renamed some files and added a lot of new docs

…afilter-1

coderabbitai

Actionable comments posted: 5

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6613d6c and 2079250.

📒 Files selected for processing (13)

presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpConnector.java (1 hunks)
presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpMetadataFilterProvider.java (0 hunks)
presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpModule.java (2 hunks)
presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpPlanOptimizer.java (2 hunks)
presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpPlanOptimizerProvider.java (1 hunks)
presto-clp/src/main/java/com/facebook/presto/plugin/clp/metadata/filter/ClpMetadataDatabaseSpecificDeserializer.java (1 hunks)
presto-clp/src/main/java/com/facebook/presto/plugin/clp/metadata/filter/ClpMetadataFilter.java (1 hunks)
presto-clp/src/main/java/com/facebook/presto/plugin/clp/metadata/filter/ClpMetadataFilterProvider.java (1 hunks)
presto-clp/src/main/java/com/facebook/presto/plugin/clp/metadata/filter/ClpMySqlMetadataFilterProvider.java (1 hunks)
presto-clp/src/test/java/com/facebook/presto/plugin/clp/metadata/filter/TestClpMetadataFilterConfigCommon.java (5 hunks)
presto-clp/src/test/java/com/facebook/presto/plugin/clp/metadata/filter/TestClpMySqlMetadataFilterConfig.java (1 hunks)
presto-clp/src/test/resources/test-mysql-metadata-filter.json (1 hunks)
presto-docs/src/main/sphinx/connector/clp.rst (2 hunks)

🧠 Learnings (11)

📓 Common learnings

Learnt from: anlowee
PR: y-scope/presto#7
File: .github/workflows/hive-tests.yml:33-35
Timestamp: 2025-06-04T14:50:43.554Z
Learning: In the y-scope/presto repository, GitHub workflows are extended from OSS Presto and should maintain consistent formatting with the upstream patterns, including YAML indentation styles for workflow steps.

Learnt from: wraymo
PR: y-scope/presto#15
File: presto-clp/src/main/java/com/facebook/presto/plugin/clp/metadata/ClpMetadataProvider.java:22-33
Timestamp: 2025-06-13T12:56:06.325Z
Learning: `ClpMetadataProvider` is instantiated only once and used solely by the Presto coordinator, so concurrency/thread-safety guarantees are unnecessary.

presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpModule.java (1)

Learnt from: wraymo
PR: #15
File: presto-clp/src/main/java/com/facebook/presto/plugin/clp/metadata/ClpMetadataProvider.java:22-33
Timestamp: 2025-06-13T12:56:06.325Z
Learning: ClpMetadataProvider is instantiated only once and used solely by the Presto coordinator, so concurrency/thread-safety guarantees are unnecessary.

presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpPlanOptimizer.java (1)

Learnt from: wraymo
PR: #15
File: presto-clp/src/main/java/com/facebook/presto/plugin/clp/metadata/ClpMetadataProvider.java:22-33
Timestamp: 2025-06-13T12:56:06.325Z
Learning: ClpMetadataProvider is instantiated only once and used solely by the Presto coordinator, so concurrency/thread-safety guarantees are unnecessary.

presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpConnector.java (1)

Learnt from: wraymo
PR: #15
File: presto-clp/src/main/java/com/facebook/presto/plugin/clp/metadata/ClpMetadataProvider.java:22-33
Timestamp: 2025-06-13T12:56:06.325Z
Learning: ClpMetadataProvider is instantiated only once and used solely by the Presto coordinator, so concurrency/thread-safety guarantees are unnecessary.

presto-clp/src/main/java/com/facebook/presto/plugin/clp/metadata/filter/ClpMetadataDatabaseSpecificDeserializer.java (1)

Learnt from: wraymo
PR: #15
File: presto-clp/src/main/java/com/facebook/presto/plugin/clp/metadata/ClpMetadataProvider.java:22-33
Timestamp: 2025-06-13T12:56:06.325Z
Learning: ClpMetadataProvider is instantiated only once and used solely by the Presto coordinator, so concurrency/thread-safety guarantees are unnecessary.

presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpPlanOptimizerProvider.java (1)

Learnt from: wraymo
PR: #15
File: presto-clp/src/main/java/com/facebook/presto/plugin/clp/metadata/ClpMetadataProvider.java:22-33
Timestamp: 2025-06-13T12:56:06.325Z
Learning: ClpMetadataProvider is instantiated only once and used solely by the Presto coordinator, so concurrency/thread-safety guarantees are unnecessary.

presto-clp/src/main/java/com/facebook/presto/plugin/clp/metadata/filter/ClpMetadataFilter.java (1)

Learnt from: wraymo
PR: #15
File: presto-clp/src/main/java/com/facebook/presto/plugin/clp/metadata/ClpMetadataProvider.java:22-33
Timestamp: 2025-06-13T12:56:06.325Z
Learning: ClpMetadataProvider is instantiated only once and used solely by the Presto coordinator, so concurrency/thread-safety guarantees are unnecessary.

presto-clp/src/test/java/com/facebook/presto/plugin/clp/metadata/filter/TestClpMetadataFilterConfigCommon.java (1)

Learnt from: wraymo
PR: #15
File: presto-clp/src/main/java/com/facebook/presto/plugin/clp/metadata/ClpMetadataProvider.java:22-33
Timestamp: 2025-06-13T12:56:06.325Z
Learning: ClpMetadataProvider is instantiated only once and used solely by the Presto coordinator, so concurrency/thread-safety guarantees are unnecessary.

presto-clp/src/test/java/com/facebook/presto/plugin/clp/metadata/filter/TestClpMySqlMetadataFilterConfig.java (1)

Learnt from: wraymo
PR: #15
File: presto-clp/src/main/java/com/facebook/presto/plugin/clp/metadata/ClpMetadataProvider.java:22-33
Timestamp: 2025-06-13T12:56:06.325Z
Learning: ClpMetadataProvider is instantiated only once and used solely by the Presto coordinator, so concurrency/thread-safety guarantees are unnecessary.

presto-clp/src/main/java/com/facebook/presto/plugin/clp/metadata/filter/ClpMetadataFilterProvider.java (1)

Learnt from: wraymo
PR: #15
File: presto-clp/src/main/java/com/facebook/presto/plugin/clp/metadata/ClpMetadataProvider.java:22-33
Timestamp: 2025-06-13T12:56:06.325Z
Learning: ClpMetadataProvider is instantiated only once and used solely by the Presto coordinator, so concurrency/thread-safety guarantees are unnecessary.

presto-clp/src/main/java/com/facebook/presto/plugin/clp/metadata/filter/ClpMySqlMetadataFilterProvider.java (1)

Learnt from: wraymo
PR: #15
File: presto-clp/src/main/java/com/facebook/presto/plugin/clp/metadata/ClpMetadataProvider.java:22-33
Timestamp: 2025-06-13T12:56:06.325Z
Learning: ClpMetadataProvider is instantiated only once and used solely by the Presto coordinator, so concurrency/thread-safety guarantees are unnecessary.

💤 Files with no reviewable changes (1)

presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpMetadataFilterProvider.java

🧰 Additional context used

🧠 Learnings (11)

📓 Common learnings

Learnt from: anlowee
PR: y-scope/presto#7
File: .github/workflows/hive-tests.yml:33-35
Timestamp: 2025-06-04T14:50:43.554Z
Learning: In the y-scope/presto repository, GitHub workflows are extended from OSS Presto and should maintain consistent formatting with the upstream patterns, including YAML indentation styles for workflow steps.

Learnt from: wraymo
PR: y-scope/presto#15
File: presto-clp/src/main/java/com/facebook/presto/plugin/clp/metadata/ClpMetadataProvider.java:22-33
Timestamp: 2025-06-13T12:56:06.325Z
Learning: `ClpMetadataProvider` is instantiated only once and used solely by the Presto coordinator, so concurrency/thread-safety guarantees are unnecessary.