feat: Add split type to ClpSplit to prepare to support searching CLP IR files; Update to the latest commit of y-scope/velox:presto-0.293-clp-connector. - #50
Conversation
…ict with ColumnHandle class Type
WalkthroughA new Changes
Sequence Diagram(s)sequenceDiagram
participant SplitProvider as ClpMySqlSplitProvider
participant ClpSplit as ClpSplit (Java)
participant Protocol as ClpSplit (C++)
participant VeloxConnector as ClpPrestoToVeloxConnector
SplitProvider->>ClpSplit: new ClpSplit(path, query, SplitType.ARCHIVE)
ClpSplit->>Protocol: Serialize with type (via JSON)
Protocol->>VeloxConnector: Deserialize, provide type
VeloxConnector->>VeloxConnector: Construct ClpConnectorSplit(..., type)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15–20 minutes Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
🔇 Additional comments (6)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpSplit.java(3 hunks)presto-clp/src/main/java/com/facebook/presto/plugin/clp/split/ClpMySqlSplitProvider.java(2 hunks)presto-native-execution/presto_cpp/main/connectors/PrestoToVeloxConnector.cpp(1 hunks)presto-native-execution/presto_cpp/presto_protocol/connector/clp/presto_protocol_clp.cpp(2 hunks)presto-native-execution/presto_cpp/presto_protocol/connector/clp/presto_protocol_clp.h(1 hunks)presto-native-execution/velox(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: `clpmetadataprovider` is instantiated only once and used solely by the presto coordinator, so concur...
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.
Applied to files:
presto-clp/src/main/java/com/facebook/presto/plugin/clp/split/ClpMySqlSplitProvider.java
🧬 Code Graph Analysis (1)
presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpSplit.java (2)
presto-native-execution/presto_cpp/presto_protocol/connector/clp/presto_protocol_clp.cpp (1)
ClpSplit(108-110)presto-native-execution/presto_cpp/presto_protocol/connector/clp/presto_protocol_clp.h (1)
SplitType(52-55)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: test (17.0.13, :presto-tests -P ci-only-aggregation-queries)
- GitHub Check: test (17.0.13, :presto-tests -P ci-only-distributed-non-hash-gen)
- GitHub Check: test (8.0.442, :presto-main-base)
- GitHub Check: test (8.0.442, :presto-tests -P ci-only-plan-determinism)
- GitHub Check: test (17.0.13, :presto-tests -P presto-tests-general)
- GitHub Check: test (8.0.442, :presto-tests -P ci-only-distributed-queries)
- GitHub Check: test (8.0.442, :presto-tests -P ci-only-local-queries)
- GitHub Check: test (8.0.442, :presto-tests -P ci-only-aggregation-queries)
- GitHub Check: test (8.0.442, :presto-tests -P presto-tests-general)
- GitHub Check: test (8.0.442, :presto-tests -P presto-tests-execution-memory)
- GitHub Check: prestocpp-linux-build-for-test
- GitHub Check: maven-checks (17.0.13)
- GitHub Check: maven-checks (8.0.442)
- GitHub Check: prestissimo-worker-images-build
🔇 Additional comments (9)
presto-clp/src/main/java/com/facebook/presto/plugin/clp/split/ClpMySqlSplitProvider.java (2)
32-32: LGTM!The static import for
ARCHIVEis appropriately added to support the updatedClpSplitconstructor.
85-85: LGTM!The constructor call correctly specifies
ARCHIVEas the split type, which is appropriate since this provider is specifically handling archive-based splits from the MySQL metadata.presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpSplit.java (3)
37-37: LGTM!The
typefield is properly declared, constructor parameter is correctly annotated for JSON serialization, and the field initialization includes appropriate null validation.Also applies to: 42-43, 47-47
62-66: LGTM!The getter method is properly implemented with JSON annotation for serialization support.
86-90: LGTM!The
SplitTypeenum is well-defined with meaningful valuesARCHIVEandIRthat clearly distinguish between the two dataset types as intended.presto-native-execution/presto_cpp/presto_protocol/connector/clp/presto_protocol_clp.h (2)
52-55: LGTM!The
SplitTypeenum class is properly defined with the same values as the Java counterpart (ARCHIVE,IR), and the JSON serialization function declarations follow the established pattern in this codebase.
60-60: LGTM!The
typefield is appropriately added to theClpSplitstruct with proper default initialization. The default value{}will initialize to the first enum value (ARCHIVE), which is reasonable.presto-native-execution/presto_cpp/main/connectors/PrestoToVeloxConnector.cpp (1)
1567-1570: LGTM!The constructor call correctly includes the split type by casting the enum to an integer, which properly propagates the split type information to the Velox connector layer.
presto-native-execution/presto_cpp/presto_protocol/connector/clp/presto_protocol_clp.cpp (1)
117-117: LGTM!The
ClpSplitJSON serialization and deserialization functions are correctly updated to handle the newtypefield using the appropriate helper functions.Also applies to: 124-124
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
presto-native-execution/velox (1)
1-1: Still pointing to a raw SHA – prefer pinning to a tagged Velox release for reproducibility
Same concern as in earlier reviews: a moving commit hash hampers repeatable builds and bisects. If Velox exposes a stable tag that already contains theSplitTypework, please reference that tag instead.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
presto-native-execution/velox(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: test (17.0.13, :presto-tests -P ci-only-resource-manager)
- GitHub Check: test (17.0.13, :presto-tests -P ci-only-aggregation-queries)
- GitHub Check: test (8.0.442, :presto-tests -P presto-tests-execution-memory)
- GitHub Check: test (8.0.442, :presto-tests -P ci-only-distributed-non-hash-gen)
- GitHub Check: test (8.0.442, :presto-tests -P ci-only-aggregation-queries)
- GitHub Check: prestocpp-linux-build-for-test
- GitHub Check: maven-checks (8.0.442)
- GitHub Check: maven-checks (17.0.13)
- GitHub Check: prestissimo-worker-images-build
anlowee
changed the title
type in ClpSplit Java class and update the Presto protocol code and Velox.type in ClpSplit Java class; update the Presto protocol code.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpSplit.java(3 hunks)presto-clp/src/main/java/com/facebook/presto/plugin/clp/split/ClpMySqlSplitProvider.java(2 hunks)presto-native-execution/velox(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: `clpmetadataprovider` is instantiated only once and used solely by the presto coordinator, so concur...
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.
Applied to files:
presto-clp/src/main/java/com/facebook/presto/plugin/clp/split/ClpMySqlSplitProvider.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: test (17.0.13, :presto-tests -P ci-only-aggregation-queries)
- GitHub Check: test (17.0.13, :presto-main)
- GitHub Check: test (17.0.13, :presto-tests -P ci-only-plan-determinism)
- GitHub Check: test (17.0.13, :presto-main-base)
- GitHub Check: test (8.0.442, :presto-main)
- GitHub Check: test (17.0.13, :presto-tests -P ci-only-local-queries)
- GitHub Check: test (17.0.13, :presto-tests -P presto-tests-execution-memory)
- GitHub Check: test (8.0.442, :presto-main-base)
- GitHub Check: test (17.0.13, :presto-tests -P ci-only-tpch-distributed-queries)
- GitHub Check: test (8.0.442, :presto-tests -P ci-only-aggregation-queries)
- GitHub Check: test (8.0.442, :presto-tests -P ci-only-distributed-non-hash-gen)
- GitHub Check: test (8.0.442, :presto-tests -P ci-only-distributed-queries)
- GitHub Check: test (8.0.442, :presto-tests -P ci-only-local-queries)
- GitHub Check: test (8.0.442, :presto-tests -P ci-only-tpch-distributed-queries)
- GitHub Check: test (8.0.442, :presto-tests -P presto-tests-general)
- GitHub Check: test (8.0.442, :presto-tests -P presto-tests-execution-memory)
- GitHub Check: prestissimo-worker-images-build
- GitHub Check: maven-checks (8.0.442)
- GitHub Check: maven-checks (17.0.13)
- GitHub Check: prestocpp-linux-build-for-test
🔇 Additional comments (8)
presto-native-execution/velox (1)
1-1: SplitType API changes present in Velox commit 420a46aed
Verified that the commit includes all requiredSplitTypemodifications—enum naming, serialization, and call-sites—so no further SplitType updates are needed before merging.• velox/connectors/clp/ClpConnectorSplit.h – updated
enum class SplitType { kArchive, kIr };, adjusted constructor andtoString()to use"Ir".
• velox/connectors/clp/tests/ClpConnectorTest.cpp – test call-sites amended to match the newSplitTypeAPI.presto-clp/src/main/java/com/facebook/presto/plugin/clp/split/ClpMySqlSplitProvider.java (2)
32-32: LGTM! Clean static import for enum constant.The static import for
ARCHIVEfollows Java best practices and improves code readability.
85-85: LGTM! Correct integration with new ClpSplit constructor.The addition of
ARCHIVEas the split type parameter is appropriate for a MySQL split provider that handles archive data, as evidenced by the archive-related table queries in this class.presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpSplit.java (5)
36-36: LGTM! Proper field declaration with immutability.The
typefield is correctly declared asfinal, maintaining the immutable nature of theClpSplitclass.
42-42: LGTM! Proper JSON serialization annotation.The
@JsonProperty("type")annotation correctly enables JSON serialization for the new type parameter.
46-46: LGTM! Appropriate null validation.The null check with
requireNonNullensures the split type cannot be null, maintaining data integrity with a clear error message.
56-60: LGTM! Standard getter with proper JSON annotation.The
getType()method follows Java conventions and includes the necessary@JsonPropertyannotation for JSON serialization.
86-90: LGTM! Well-designed enum for split type categorization.The
SplitTypeenum withARCHIVEandIRvalues clearly serves the purpose of distinguishing dataset types for search operations as outlined in the PR objectives.
67d229a to
01a4b9a
Compare
| public Map<String, String> getInfo() | ||
| { | ||
| return ImmutableMap.of("path", path, "kqlQuery", kqlQuery.orElse("<null>")); | ||
| return ImmutableMap.of("path", path, "kqlQuery", "type", type.toString(), kqlQuery.orElse("<null>")); |
There was a problem hiding this comment.
As the rabbit said, the order has an error.
There was a problem hiding this comment.
yea, will fix after velox PR merged
There was a problem hiding this comment.
Fixed and bumped the Velox version
01a4b9a to
eb53191
Compare
kirkrodrigues
left a comment
kirkrodrigues
left a comment
There was a problem hiding this comment.
For the PR title, how about:
feat: Add split type to `ClpSplit` to prepare to support searching CLP IR files; Update to the latest commit of y-scope/velox:presto-0.293-clp-connector.
type in ClpSplit Java class; update the Presto protocol code.ClpSplit to prepare to support searching CLP IR files; Update to the latest commit of *y-scope/velox:presto-0.293-clp-connector*.
anlowee
changed the title
ClpSplit to prepare to support searching CLP IR files; Update to the latest commit of *y-scope/velox:presto-0.293-clp-connector*.ClpSplit to prepare to support searching CLP IR files; Update to the latest commit of y-scope/velox:presto-0.293-clp-connector.
c8ac602
into
y-scope:release-0.293-clp-connector
3 participants
Description
We added
typeproperty stored in theClpSplitand nested enum classSplitType(We cannot name itTypebecause it will conflict withColumnHandle'sTypebecause there is no way to config this remapping in protocol's YAML according to the doc). This property is to discern whether the dataset we want to conduct search is IR files or archives. We can refer to this commit.Checklist
breaking change.
Validation performed
Set a breakpoint at
ClpDataSource.cpp:addSplit()to check if the type was passed and parsed correctly.Summary by CodeRabbit