[native] Add CLP connector #4
Conversation
WalkthroughThe changes update build configurations and introduce a new CLP connector. The modifications in CMake files add the "-no-pie" option and update library dependencies. In the server code, the CLP connector is registered and the connector factory is conditionally added. New files, including headers, source files, JSON schemas, Mustache templates, and YAML configurations, provide CLP protocol definitions and serialization support. Additionally, special include files for serialization of CLP components and protocol integration files are added. Finally, the subproject commit for Velox has been updated. Changes
Sequence Diagram(s)sequenceDiagram
participant Server as PrestoServer
participant CLP as ClpPrestoToVeloxConnector
participant Registry as ConnectorFactoryRegistry
Server->>CLP: Instantiate new CLP connector
Server->>Registry: Check if CLP factory exists
alt Factory not registered
Server->>Registry: Register ClpConnectorFactory
else Factory already registered
Registry-->>Server: Factory exists
end
✨ Finishing Touches
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:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (14)
presto-native-execution/presto_cpp/presto_protocol/connector/clp/presto_protocol_clp.json (2)
32-37: Consistent naming for boolean types.
Thefield_typeis declared asboolean, whilefield_textisbool. For clarity and maintainability, consider unifying the naming in generated code to consistently usebool. This helps avoid confusion for future contributors.
44-114: Add usage examples or documentation blocks.
ClpSplit,ClpTableHandle, andClpTableLayoutHandleare defined clearly, but some developers may benefit from usage examples or inline documentation describing expected values, especially for fields likequerythat can be optional.presto-native-execution/presto_cpp/presto_protocol/connector/clp/presto_protocol-json-cpp.mustache (2)
73-93: Consider switching to a faster lookup for enum serialization.
The current approach uses a linear search through a static array. Although this is typically acceptable for small enumerations, consider using a hash-based map if enums grow significantly, to provide faster lookups for serialization and deserialization.
107-142: Raise a more descriptive error for abstract mismatches.
When an unrecognised or invalid_typeis encountered duringfrom_json, a fairly generic error is thrown. Providing more descriptive guidance or referencing valid_typevalues could aid debugging for future integrations.presto-native-execution/presto_cpp/presto_protocol/connector/clp/ClpConnectorProtocol.h (1)
23-27: Document plans for unimplemented functionality.Many template parameters are set to
NotImplemented, indicating features that aren't currently implemented. Consider adding documentation about which features are planned for future implementation versus those that will remain unimplemented.presto-native-execution/presto_cpp/main/tests/CMakeLists.txt (1)
26-26: Consider the security implications of using -no-pie.The
-no-pieflag disables Position Independent Executable, which is a security feature that helps mitigate certain types of attacks. While this may be necessary for linking compatibility, it should be documented why this option is required.+# Disable PIE to address linking issues with the CLP connector components target_link_options(presto_server_test PRIVATE "-no-pie")presto-native-execution/presto_cpp/main/CMakeLists.txt (1)
102-102: Consider the security implications of using -no-pie.Similar to the test configuration, disabling Position Independent Executable (PIE) might expose the application to certain security vulnerabilities. If possible, consider addressing the underlying linking issues that necessitate this flag.
+# Disable PIE to address linking issues with the CLP connector components target_link_options(presto_server PRIVATE "-no-pie")presto-native-execution/presto_cpp/presto_protocol/connector/clp/special/ClpColumnHandle.hpp.inc (1)
18-33: Be cautious with dynamic_cast in operator< method.The
operator<implementation usesdynamic_castwithout checking if the cast is successful. This could potentially throw astd::bad_castexception if an object of a different type is compared.- bool operator<(const ColumnHandle& o) const override { - return columnName < dynamic_cast<const ClpColumnHandle&>(o).columnName; - } + bool operator<(const ColumnHandle& o) const override { + const auto* other = dynamic_cast<const ClpColumnHandle*>(&o); + if (false == other) { + throw std::runtime_error("Cannot compare with a non-ClpColumnHandle"); + } + return columnName < other->columnName; + }Additionally, note that the constructor
ClpColumnHandle() noexceptis declared but not defined in this file. Ensure it's implemented elsewhere.presto-native-execution/presto_cpp/presto_protocol/connector/clp/presto_protocol-json-hpp.mustache (1)
25-34: Ensure comprehensive testing for JSON round-trip
Theto_json/from_jsonimplementations forClpTransactionHandleproperly push and read array elements, ensuring consistent ordering. Consider adding unit tests (or verifying auto-generated tests) to confirm that both functions work correctly under various scenarios.presto-native-execution/presto_cpp/presto_protocol/connector/clp/presto_protocol_clp.cpp (2)
25-34: Array-based JSON for ClpTransactionHandle
Storing_typeandinstancefields in a JSON array is consistent with the usage infrom_json. This approach is valid, though a JSON object structure might be more descriptive. If you anticipate additional fields in the future, consider switching to an object-based schema for clarity.
110-156: Proper handling of table and layout handles
TheClpTableHandleandClpTableLayoutHandlehandle standard fields (schemaTableName,query) thoroughly. Confirm that these are tested in any integration tests that rely on JSON-based configuration or dynamic handle retrieval.presto-native-execution/presto_cpp/presto_protocol/connector/clp/presto_protocol_clp.h (3)
32-45: Duplicate License Header DetectedA second license header appears starting at line 32. Please verify if this repetition is intentional; if not, consider consolidating the license headers to avoid redundancy.
59-61: Dynamic Cast in Operator<The overridden
operator<employs adynamic_castto convert the base reference to aClpColumnHandle. Ensure that the compared objects are always of the correct type so that the cast does not fail at runtime. It may be advisable to add runtime type verification or consider a safer casting approach if feasible.
66-76: ClpSplit Structure ReviewThe declaration of
ClpSplit, including its members and the noexcept constructor, is clear. The use ofstd::shared_ptr<String>for thequeryfield is notable; please verify that this design choice is intentional and offers a benefit over a simpleStringmember.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
presto-native-execution/presto_cpp/main/CMakeLists.txt(1 hunks)presto-native-execution/presto_cpp/main/PrestoServer.cpp(3 hunks)presto-native-execution/presto_cpp/main/tests/CMakeLists.txt(2 hunks)presto-native-execution/presto_cpp/main/types/PrestoToVeloxConnector.cpp(2 hunks)presto-native-execution/presto_cpp/main/types/PrestoToVeloxConnector.h(1 hunks)presto-native-execution/presto_cpp/main/types/tests/CMakeLists.txt(3 hunks)presto-native-execution/presto_cpp/presto_protocol/Makefile(1 hunks)presto-native-execution/presto_cpp/presto_protocol/connector/clp/ClpConnectorProtocol.h(1 hunks)presto-native-execution/presto_cpp/presto_protocol/connector/clp/presto_protocol-json-cpp.mustache(1 hunks)presto-native-execution/presto_cpp/presto_protocol/connector/clp/presto_protocol-json-hpp.mustache(1 hunks)presto-native-execution/presto_cpp/presto_protocol/connector/clp/presto_protocol_clp.cpp(1 hunks)presto-native-execution/presto_cpp/presto_protocol/connector/clp/presto_protocol_clp.h(1 hunks)presto-native-execution/presto_cpp/presto_protocol/connector/clp/presto_protocol_clp.json(1 hunks)presto-native-execution/presto_cpp/presto_protocol/connector/clp/presto_protocol_clp.yml(1 hunks)presto-native-execution/presto_cpp/presto_protocol/connector/clp/special/ClpColumnHandle.hpp.inc(1 hunks)presto-native-execution/presto_cpp/presto_protocol/connector/clp/special/ClpTransactionHandle.cpp.inc(1 hunks)presto-native-execution/presto_cpp/presto_protocol/connector/clp/special/ClpTransactionHandle.hpp.inc(1 hunks)presto-native-execution/presto_cpp/presto_protocol/presto_protocol.cpp(1 hunks)presto-native-execution/presto_cpp/presto_protocol/presto_protocol.h(1 hunks)presto-native-execution/velox(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
presto-native-execution/presto_cpp/presto_protocol/presto_protocol.cpppresto-native-execution/presto_cpp/main/types/PrestoToVeloxConnector.cpppresto-native-execution/presto_cpp/main/PrestoServer.cpppresto-native-execution/presto_cpp/presto_protocol/connector/clp/presto_protocol_clp.cpp
🧬 Code Definitions (3)
presto-native-execution/presto_cpp/main/types/PrestoToVeloxConnector.h (1)
presto-native-execution/presto_cpp/main/SystemConnector.h (3)
catalogId(185-188)column(190-192)tableHandle(194-201)
presto-native-execution/presto_cpp/main/types/PrestoToVeloxConnector.cpp (3)
presto-native-execution/presto_cpp/main/types/PrestoToVeloxSplit.cpp (2)
toVeloxSplit(22-47)toVeloxSplit(22-23)presto-native-execution/presto_cpp/main/types/PrestoToVeloxConnector.h (15)
catalogId(46-49)catalogId(117-120)catalogId(169-172)catalogId(196-199)catalogId(223-226)column(52-54)column(122-124)column(174-176)column(201-203)column(228-230)tableHandle(57-64)tableHandle(126-133)tableHandle(178-185)tableHandle(205-212)tableHandle(232-239)presto-native-execution/presto_cpp/main/SystemConnector.h (3)
catalogId(185-188)column(190-192)tableHandle(194-201)
presto-native-execution/presto_cpp/presto_protocol/connector/clp/presto_protocol_clp.cpp (1)
presto-native-execution/presto_cpp/presto_protocol/connector/clp/presto_protocol_clp.h (4)
ClpColumnHandle(57-57)ClpSplit(72-72)ClpTableHandle(81-81)ClpTableLayoutHandle(91-91)
🔇 Additional comments (38)
presto-native-execution/presto_cpp/presto_protocol/connector/clp/presto_protocol_clp.json (2)
3-4: Confirm the generation workflow.
These lines state that the file is generated and should not be edited manually. Please confirm that there is a secure and reliable pipeline for regenerating this file, to avoid inadvertent manual modifications that could be overwritten.Would you like assistance in validating the generator script or verifying its usage via a build command or documentation lookup?
7-7: Caution with dynamic_cast in operator<.
Within thehincblock,operator<(...)relies ondynamic_castto downcast the base class reference. This may produce unexpected behaviour if the cast fails, potentially throwingstd::bad_castat runtime. Ensure that all instances passed tooperator<are indeedClpColumnHandle.Would you like me to generate a script to locate and verify all usage sites of
ClpColumnHandle::operator<to confirm proper usage?presto-native-execution/presto_cpp/presto_protocol/connector/clp/presto_protocol-json-cpp.mustache (1)
46-71: Ensure_typeusage matches the schema.
When assigningp._typeinside the constructors and from JSON, confirm that_typealigns with the expected class or subclass key. Mismatched or missing_typeassignments can lead to parsing or type lookup errors in the rest of the pipeline.presto-native-execution/velox (1)
1-1: Subproject Commit Update VerificationThe subproject commit has been updated to
16932ecfc32b94734640e44d03cbb1e5361c50c6. Please ensure that this update is fully compatible with the CLP connector native code integration and does not introduce any regressions or compatibility issues.presto-native-execution/presto_cpp/presto_protocol/presto_protocol.h (2)
19-19: Include for new CLP connector protocol added successfully.The include directive for the CLP protocol header file follows the same pattern as the other connector inclusions and is properly placed.
17-19: Consider the implications of adding to a deprecated file.This file is marked as deprecated at line 17, yet new functionality is being added. While this approach may be necessary for transitional purposes, ensure there's a plan for eventually migrating this functionality when the file is removed.
presto-native-execution/presto_cpp/presto_protocol/presto_protocol.cpp (2)
18-18: Include for new CLP connector implementation added successfully.The include statement follows the established pattern used for other connector implementations in this file.
16-18: Consider the implications of adding to a deprecated file.Similar to the header file, this implementation file is marked as deprecated, yet new functionality is being added. Ensure there's a migration plan for when these files are eventually removed.
presto-native-execution/presto_cpp/presto_protocol/connector/clp/special/ClpTransactionHandle.cpp.inc (3)
15-17: Good documentation of special case handling.The comment clearly explains why this type requires special handling (Java counterpart is an enum), which is helpful for future maintenance.
20-24: JSON serialization implementation looks correct.The serialization method properly captures the structure as a JSON array containing the _type and instance values.
26-29: JSON deserialization implementation looks correct.The deserialization method properly extracts values from the JSON array and assigns them to the appropriate struct members.
presto-native-execution/presto_cpp/presto_protocol/connector/clp/ClpConnectorProtocol.h (1)
19-28: Type alias defined with appropriate template parameters.The
ClpConnectorProtocoltype is properly defined as an alias forConnectorProtocolTemplatewith specific CLP connector types.presto-native-execution/presto_cpp/main/types/tests/CMakeLists.txt (1)
29-29: These changes integrate the CLP connector into the test framework.The addition of
velox_clp_connectorto all three test targets is consistent and ensures that the CLP connector functionality is properly tested across different components. This is a well-structured approach to extending the testing framework to include the new connector.Also applies to: 66-66, 98-98
presto-native-execution/presto_cpp/main/tests/CMakeLists.txt (1)
48-48: Consistent integration of the CLP connector in the test suite.Adding
velox_clp_connectorto the test executable ensures that the tests can properly exercise the CLP connector functionality, maintaining consistency with the main executable changes.presto-native-execution/presto_cpp/main/CMakeLists.txt (1)
109-109:❓ Verification inconclusive
CLP connector integration looks appropriate.
The addition of
velox_clp_connectorto the link libraries aligns with the integration goals of this PR. However, note that the existing TODO comment suggests there are unresolved linking issues that might need to be addressed in a future PR.
🏁 Script executed:
#!/bin/bash # Verify that no references to TpchTableHandle remain unresolved in the codebase # after replacing velox_tpch_connector with velox_clp_connector # First, search for any imports or uses of TpchTableHandle in source files echo "Searching for TpchTableHandle references in the codebase:" rg "TpchTableHandle" --type cpp --type hpp # Then check if there are any residual TPCH connector initializations that # might need to be updated to use CLP connector instead echo "Searching for TPCH connector registrations that might need updating:" rg "registerConnector.*(\"tpch\")" --type cppLength of output: 388
Attention: Manual Verification on Unresolved References Required
The CLP connector integration appears correct, and the linking adjustment at line 109 in
presto-native-execution/presto_cpp/main/CMakeLists.txtproperly includesvelox_clp_connector. However, the automated search for any remainingTpchTableHandlereferences did not yield a conclusive output due to an error with the file type parameter (i.e. "unrecognized file type: hpp"). This means that while the current integration meets the PR's goals, you should manually verify that:
- No unresolved
TpchTableHandlereferences remain in the codebase.- The existing TODO-comment regarding linking issues is revisited in future updates.
presto-native-execution/presto_cpp/presto_protocol/connector/clp/special/ClpTransactionHandle.hpp.inc (1)
1-28:❓ Verification inconclusive
The CLP transaction handle implementation looks good, but ensure complete implementation.
This header file properly defines the CLP transaction handle structure with appropriate JSON serialization declarations. The comment about this corresponding to a Java enum is useful documentation.
A few considerations:
- Make sure the
to_jsonandfrom_jsonfunctions are properly implemented in a corresponding cpp file- Since this struct corresponds to a Java enum, ensure that the serialization/deserialization correctly handles this difference in representation between languages
Otherwise, the code structure is clean and follows the project's patterns.
🏁 Script executed:
#!/bin/bash # Verify that the to_json and from_json functions for ClpTransactionHandle are # implemented in a corresponding cpp file echo "Searching for ClpTransactionHandle to_json/from_json implementations:" rg "void to_json\(json& j, const ClpTransactionHandle& p\)" --type cpp echo "Checking for associated CPP files that might contain the implementation:" fd -e cpp --exec grep -l "ClpTransactionHandle" {} \;Length of output: 809
CLP Transaction Handle – Verify JSON Deserialization
- The
to_jsonfunction is implemented inpresto-native-execution/presto_cpp/presto_protocol/connector/clp/presto_protocol_clp.cppas expected.- Please verify that the corresponding
from_jsonfunction is also implemented, ensuring it correctly handles the transformation, particularly given that this struct corresponds to a Java enum.- Confirm that both serialization and deserialization account for any differences in enum representation between C++ and Java.
Overall, the structure and documentation look clean; just ensure that the complete JSON conversion (both
to_jsonandfrom_json) is properly addressed.presto-native-execution/presto_cpp/main/PrestoServer.cpp (3)
51-51: Include for the CLP connector has been properly added.The include for the CLP connector header follows the existing pattern in the file.
275-276: New CLP connector registration correctly implemented.The registration of
ClpPrestoToVeloxConnectorfollows the same pattern as the other connector registrations (Hive, Iceberg, Tpch) in this file.
1189-1193: Proper conditional registration of the CLP connector factory.This code correctly checks if the ClpConnectorFactory is already registered before registering it, following the pattern used for other connector factories. The implementation ensures that the factory is registered only once.
presto-native-execution/presto_cpp/presto_protocol/connector/clp/presto_protocol_clp.yml (1)
13-39: The CLP connector protocol definition looks good.The YAML configuration correctly defines the abstract classes and their CLP-specific subclasses, following the same pattern as other connectors in the system. The class hierarchy is properly specified with appropriate superclasses.
presto-native-execution/presto_cpp/presto_protocol/Makefile (3)
48-53: Build steps for CLP connector correctly added.The additions follow the same pattern used for other connectors (Hive, Iceberg, Tpch) and properly generate the required files for the CLP connector using the same tools and process.
60-60: JSON generation for CLP connector properly added.The command to generate the JSON file for the CLP connector follows the same pattern as the other connectors.
67-67: Protobuf generation for CLP connector correctly added.The command to generate the protobuf file for the CLP connector follows the same pattern as the other connectors.
presto-native-execution/presto_cpp/presto_protocol/connector/clp/presto_protocol-json-hpp.mustache (2)
1-13: Licence header is clearly defined
The Apache licence header is properly included.
36-69: Template expansions appear correct
These template expansions for structs and enums look coherent and aligned with the rest of the code. Ensure any optional fields or inherited classes are properly handled into_json/from_jsonif you introduce them later.presto-native-execution/presto_cpp/main/types/PrestoToVeloxConnector.h (1)
217-243: New CLP connector class is well-structured
TheClpPrestoToVeloxConnectorclass integrates seamlessly with the existing framework. Its constructor and final methods match the base class interface and follow the established pattern from other connectors (e.g.,TpchPrestoToVeloxConnector,HivePrestoToVeloxConnector). Ensure you add proper coverage in unit tests or integration tests to validate the CLP connector functionality (split creation, table handle generation, etc.).presto-native-execution/presto_cpp/presto_protocol/connector/clp/presto_protocol_clp.cpp (3)
1-19: Generated file structure verified
This file is auto-generated, and it properly includes licensing and disclaimers. No issues found with the present formatting or versioning statements.
37-73: ClpColumnHandle covers all needed fields
Including properties likecolumnName,originalColumnName,columnTypeandnullableproperly in JSON ensures robust serialization. Continue maintaining the same field names on both sides ofto_jsonandfrom_jsonto avoid schema mismatches.
76-108: ClpSplit design aligns with the pattern
The_typefield is correctly assigned "clp", consistent with the rest of the code. DeserializingarchivePathandqueryfrom JSON objects ensures these fields are properly mapped. Keep an eye on edge cases such as empty strings forarchivePath.presto-native-execution/presto_cpp/main/types/PrestoToVeloxConnector.cpp (3)
16-16: Including ClpConnectorProtocol
The explicit inclusion of"ClpConnectorProtocol.h"is correct for instantiating the CLP-specific protocol in thecreateConnectorProtocolimplementation.
22-24: CLP connector headers integrated
The new includes forClpColumnHandle.h,ClpConnectorSplit.h, andClpTableHandle.hlook consistent, matching the usage in the updated methods below.
1560-1610: CLP connector methods
toVeloxSplit: Properly casts toClpSplitand creates aClpConnectorSplit. Consider verifying that an emptyarchivePathorqueryis acceptable to the downstream logic.toVeloxColumnHandle: UsesClpColumnHandle, consistent with the pattern in other connectors. The dynamic_cast check ensures correct type usage.toVeloxTableHandle: Extracts theClpTableLayoutHandleand returns aClpTableHandle. Consider addressing optional or missing fields if your future protocols evolve.createConnectorProtocol: ReturnsClpConnectorProtocolto register with the dispatcher.Overall, the new methods are coherent with the rest of the system.
presto-native-execution/presto_cpp/presto_protocol/connector/clp/presto_protocol_clp.h (6)
1-2: Auto-Generated File NoticeThese lines indicate that the file is auto-generated. Manual changes are discouraged.
25-31: ClpTransactionHandle Structure ReviewThe declaration for
ClpTransactionHandleis straightforward and conforms to the existing style. Its member initialization is clear and appropriate.
50-56: ClpColumnHandle Member Declaration – Type QueryThe members are correctly declared; however, the type
booleanused for thenullablemember (line 55) is non-standard in C++. Please verify whetherbooleanis a defined type alias in the project context. If not, it may be preferable to use the standardbool.
63-64: JSON Function Declarations for ClpColumnHandleThe
to_jsonandfrom_jsonfunction declarations follow the nlohmann::json pattern appropriately. Confirm that the corresponding implementations handle all member variables correctly.
77-85: ClpTableHandle Declaration ReviewThe
ClpTableHandlestructure is concise and its member and constructor are declared correctly. The JSON function declarations align with the pattern used elsewhere.
86-95: ClpTableLayoutHandle Structure ReviewThe
ClpTableLayoutHandleis well-defined, with its compositeClpTableHandleand the use of a shared pointer forqueryfitting the intended design. The JSON serialization function declarations are consistent with other structures.
| void to_json(json& j, const ClpTransactionHandle& p) { | ||
| j = json::array(); | ||
| j.push_back(p._type); | ||
| j.push_back(p.instance); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate JSON array length before indexing.
to_json(json& j, const ClpTransactionHandle& p) treats the JSON as an array and pushes back two elements. Conversely, from_json accesses [0] and [1] directly. Ensure the length is at least two before indexing to avoid out-of-range errors or unclear exception messages.
| # build clp connector related structs | ||
| echo "// DO NOT EDIT : This file is generated by chevron" > connector/clp/presto_protocol_clp.cpp | ||
| chevron -d connector/clp/presto_protocol_clp.json connector/clp/presto_protocol-json-cpp.mustache >> connector/clp/presto_protocol_clp.cpp | ||
| echo "// DO NOT EDIT : This file is generated by chevron" > connector/clp/presto_protocol_clp.h | ||
| chevron -d connector/clp/presto_protocol_clp.json connector/clp/presto_protocol-json-hpp.mustache >> connector/clp/presto_protocol_clp.h | ||
| clang-format -style=file -i connector/clp/presto_protocol_clp.h connector/clp/presto_protocol_clp.cpp |
There was a problem hiding this comment.
use tab instead of spaces, otherwise the Makefile cannot run
Description
This PR is a follow-up to #2 and adds Prestissimo support for the CLP connector. The native connector is now the default option and is responsible for deserializing classes from the Java coordinator, which are then used by Velox for query execution.
Motivation and Context
Check the RFC
Impact
This connector is independent from others and will not affect the existing functionalities.
Test Plan
We have performed end-to-end testing.
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.
If release note is NOT required, use:
Summary by CodeRabbit