-
Notifications
You must be signed in to change notification settings - Fork 3
feat: Add CLP connector native code #24
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.
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
Changes from all commits
4f8c525
754026b
af78eb3
7f45c1b
d2a5fd2
f118e47
cfd3ad1
bb3c1e1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -55,6 +55,7 @@ target_link_libraries( | |
| velox_abfs | ||
| velox_aggregates | ||
| velox_caching | ||
| velox_clp_connector | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Verification agent 🧩 Analysis chainUn-guarded linkage on Consider protecting the linkage behind a CMake feature/option test, e.g.: -find_package(VeloxClpConnector REQUIRED) # pseudo
-
target_link_libraries(
presto_server_lib
@@
velox_caching
- velox_clp_connector
+ $<$<TARGET_EXISTS:velox_clp_connector>:velox_clp_connector>or introduce a This pattern should be mirrored anywhere the target is linked (tests, tools). 🏁 Script executed: #!/bin/bash
# Search for references to velox_clp_connector
rg -n "velox_clp_connector" -C 3
# Search for existing PRESTO_ENABLE options to mirror pattern
rg -n "PRESTO_ENABLE" -C 3 .Length of output: 40098 Guard CLP connector linkage to avoid hard failures Introduce a dedicated CMake option and wrap every unconditional reference to
This makes CLP support opt-in and guards against undefined-target errors on legacy Velox builds. 🤖 Prompt for AI Agents |
||
| velox_common_base | ||
| velox_core | ||
| velox_dwio_common_exception | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,7 @@ | |
| #include "presto_cpp/main/connectors/arrow_flight/ArrowPrestoToVeloxConnector.h" | ||
| #endif | ||
|
|
||
| #include "velox/connectors/clp/ClpConnector.h" | ||
| #include "velox/connectors/hive/HiveConnector.h" | ||
| #include "velox/connectors/tpch/TpchConnector.h" | ||
|
|
||
|
|
@@ -45,6 +46,12 @@ void registerConnectorFactories() { | |
| std::make_shared<velox::connector::tpch::TpchConnectorFactory>()); | ||
| } | ||
|
|
||
| if (!velox::connector::hasConnectorFactory( | ||
| velox::connector::clp::ClpConnectorFactory::kClpConnectorName)) { | ||
| velox::connector::registerConnectorFactory( | ||
| std::make_shared<velox::connector::clp::ClpConnectorFactory>()); | ||
| } | ||
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Need to modify the CMakeLists.txt under this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. otherwise it cannot find the ClpConnectorFactory() when building
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The reason is we haven't updated velox code.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. velox_clp_connector was already added to another tareget. |
||
| // Register Velox connector factory for iceberg. | ||
| // The iceberg catalog is handled by the hive connector factory. | ||
| if (!velox::connector::hasConnectorFactory(kIcebergConnectorName)) { | ||
|
|
@@ -74,6 +81,8 @@ void registerConnectors() { | |
| std::make_unique<IcebergPrestoToVeloxConnector>(kIcebergConnectorName)); | ||
| registerPrestoToVeloxConnector(std::make_unique<TpchPrestoToVeloxConnector>( | ||
| velox::connector::tpch::TpchConnectorFactory::kTpchConnectorName)); | ||
| registerPrestoToVeloxConnector(std::make_unique<ClpPrestoToVeloxConnector>( | ||
| velox::connector::clp::ClpConnectorFactory::kClpConnectorName)); | ||
|
|
||
| // Presto server uses system catalog or system schema in other catalogs | ||
| // in different places in the code. All these resolve to the SystemConnector. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -52,16 +52,25 @@ presto_protocol-cpp: presto_protocol-json | |
| chevron -d connector/arrow_flight/presto_protocol_arrow_flight.json connector/arrow_flight/presto_protocol-json-hpp.mustache >> connector/arrow_flight/presto_protocol_arrow_flight.h | ||
| clang-format -style=file -i connector/arrow_flight/presto_protocol_arrow_flight.h connector/arrow_flight/presto_protocol_arrow_flight.cpp | ||
|
|
||
| presto_protocol-json: | ||
| # build clp connector related structs | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we want to make the comment have consistent indent? |
||
| 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 | ||
|
|
||
| presto_protocol-json: | ||
| ./java-to-struct-json.py --config core/presto_protocol_core.yml core/special/*.java core/special/*.inc -j | jq . > core/presto_protocol_core.json | ||
| ./java-to-struct-json.py --config connector/hive/presto_protocol_hive.yml connector/hive/special/*.inc -j | jq . > connector/hive/presto_protocol_hive.json | ||
| ./java-to-struct-json.py --config connector/iceberg/presto_protocol_iceberg.yml connector/iceberg/special/*.inc -j | jq . > connector/iceberg/presto_protocol_iceberg.json | ||
| ./java-to-struct-json.py --config connector/tpch/presto_protocol_tpch.yml connector/tpch/special/*.inc -j | jq . > connector/tpch/presto_protocol_tpch.json | ||
| ./java-to-struct-json.py --config connector/arrow_flight/presto_protocol_arrow_flight.yml connector/arrow_flight/special/*.inc -j | jq . > connector/arrow_flight/presto_protocol_arrow_flight.json | ||
| ./java-to-struct-json.py --config connector/clp/presto_protocol_clp.yml connector/clp/special/*.inc -j | jq . > connector/clp/presto_protocol_clp.json | ||
|
|
||
| presto_protocol.proto: presto_protocol-json | ||
| pystache presto_protocol-protobuf.mustache core/presto_protocol_core.json > core/presto_protocol_core.proto | ||
| pystache presto_protocol-protobuf.mustache connector/hive/presto_protocol_hive.json > connector/hive/presto_protocol_hive.proto | ||
| pystache presto_protocol-protobuf.mustache connector/iceberg/presto_protocol_iceberg.json > connector/iceberg/presto_protocol_iceberg.proto | ||
| pystache presto_protocol-protobuf.mustache connector/tpch/presto_protocol_tpch.json > connector/tpch/presto_protocol_tpch.proto | ||
| pystache presto_protocol-protobuf.mustache connector/arrow_flight/presto_protocol_arrow_flight.json > connector/arrow_flight/presto_protocol_arrow_flight.proto | ||
| pystache presto_protocol-protobuf.mustache connector/clp/presto_protocol_clp.json > connector/clp/presto_protocol_clp.proto | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| /* | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
| #pragma once | ||
| #include "presto_cpp/presto_protocol/connector/clp/presto_protocol_clp.h" | ||
| #include "presto_cpp/presto_protocol/core/ConnectorProtocol.h" | ||
|
|
||
| namespace facebook::presto::protocol::clp { | ||
| using ClpConnectorProtocol = ConnectorProtocolTemplate< | ||
| ClpTableHandle, | ||
| ClpTableLayoutHandle, | ||
| ClpColumnHandle, | ||
| NotImplemented, | ||
| NotImplemented, | ||
| ClpSplit, | ||
| NotImplemented, | ||
| ClpTransactionHandle, | ||
| NotImplemented>; | ||
| } // namespace facebook::presto::protocol::clp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Verification agent
🧩 Analysis chain
Avoid embedded back-slashes & newline chars in
DISABLED_WARNINGS; gate-Wno-restricton compiler supportThe current multiline string contains literal
\characters and newlines, so all three flags are passed to the compiler as one token ("-Wno-nullability-completeness \ -Wno-deprecated-declarations \ -Wno-restrict").That silently drops the second and third flags and can break warning control.
In addition,
-Wno-restrictis recognised by GCC but not by Clang, triggering “unknown warning option” errors on macOS CI.Proposed fix:
This treats each flag as its own list item (no stray back-slashes) and prevents build breaks on non-GCC tool-chains.
Please verify on both GCC and Clang before merging.
🏁 Script executed:
Length of output: 556
🌐 Web query:
💡 Result:
In CMake, a backslash at the end of a line inside a quoted argument acts as a line continuation character. This means the backslash (
\) and the immediately following newline are both ignored, allowing you to write long strings over multiple lines in your CMake scripts without actually inserting a newline character in the value.Example:
This outputs:
Key details:
"...").\at the end of a line (if it is an odd number of backslashes) is treated as a line continuation along with the following newline and is ignored.[2]Note:
Versions of CMake prior to 3.0 do not support this backslash-newline continuation inside quoted arguments.[2]
Summary table:
References to the CMake manual confirm this syntax:
If you need to make long argument lists or messages in CMake readable, this is the preferred technique.
Citations:
Gate
-Wno-restricton compiler supportFile: presto-native-execution/CMakeLists.txt
Lines: 33–36
CMake’s quoted-argument backslash+newline continuation removes the backslashes and newlines, so:
already produces a single string containing three space-separated flags—nothing is silently dropped.
However, Clang does not recognise
-Wno-restrictand will emit unknown-option errors on macOS CI. Please add immediately after line 36:Then verify the build on both GCC and Clang before merging.
🤖 Prompt for AI Agents