Skip to content

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.

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

Closed
wants to merge 8 commits into from
Closed

feat: Add CLP connector native code #24

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
4f8c525
Add CLP connector native code
wraymo Mar 25, 2025
754026b
update protocol code
wraymo Jul 8, 2025
af78eb3
fix storageType
wraymo Jul 8, 2025
7f45c1b
fix indent
wraymo Jul 10, 2025
d2a5fd2
advance velox
wraymo Jul 10, 2025
f118e47
add a flag
wraymo Jul 10, 2025
cfd3ad1
try fix
wraymo Jul 11, 2025
bb3c1e1
Update CMakeLists.txt
anlowee Jul 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
1 change: 1 addition & 0 deletions presto-native-execution/presto_cpp/main/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ target_link_libraries(
velox_abfs
velox_aggregates
velox_caching
velox_clp_connector
Copy link

@coderabbitai coderabbitai bot Jul 10, 2025

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

Un-guarded linkage on velox_clp_connector risks build breakage on older Velox snapshots.
Unlike Hive/TPCH, the CLP connector is still emerging and may not exist in all Velox builds distributed in the field. Linking it unconditionally will cause a hard failure (undefined target) when consumers compile against an older or stripped-down Velox build.

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 PRESTO_ENABLE_CLP_CONNECTOR option similar to the existing remote-function flag.

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 velox_clp_connector behind it (and a TARGET_EXISTS test) so that builds against older or stripped-down Velox snapshots don’t break:

  1. In presto-native-execution/CMakeLists.txt (near other PRESTO_ENABLE_* options), add:

    option(PRESTO_ENABLE_CLP_CONNECTOR "Enable CLP connector support" OFF)

if(PRESTO_ENABLE_CLP_CONNECTOR)
  # Forward to Velox
  set(VELOX_ENABLE_CLP_CONNECTOR
      ON
      CACHE BOOL "Build CLP connector support" FORCE)
  add_compile_definitions(PRESTO_ENABLE_CLP_CONNECTOR)
endif()

  2. Replace every raw velox_clp_connector entry in your target_link_libraries calls with a generator expression, for example:

      target_link_libraries(presto_server_lib
    …
-   velox_clp_connector
+   $<$<AND:$<BOOL:${PRESTO_ENABLE_CLP_CONNECTOR}>,$<TARGET_EXISTS:velox_clp_connector>>>:velox_clp_connector>
    velox_common_base
    velox_core
    …
  )

  3. Apply the same change in all CMakeLists where velox_clp_connector is linked:
    • presto-native-execution/presto_cpp/main/CMakeLists.txt (lines 55–60)
    • presto-native-execution/presto_cpp/main/tests/CMakeLists.txt (around line 50)
    • presto-native-execution/presto_cpp/main/types/tests/CMakeLists.txt (lines 30, 68, 101)
    • Any other CMakeLists discovered via rg -l velox_clp_connector.

This makes CLP support opt-in and guards against undefined-target errors on legacy Velox builds.

🤖 Prompt for AI Agents 
In presto-native-execution/presto_cpp/main/CMakeLists.txt at line 58 and related
CMakeLists files, the direct linkage to velox_clp_connector causes build
failures on older Velox versions. Fix this by adding a new CMake option
PRESTO_ENABLE_CLP_CONNECTOR in presto-native-execution/CMakeLists.txt to control
CLP connector support, forwarding it to Velox and defining a compile flag. Then,
replace all unconditional velox_clp_connector references in
target_link_libraries with a conditional generator expression that links it only
if the option is enabled and the target exists. Apply these changes consistently
across all CMakeLists files that link velox_clp_connector to make CLP support
opt-in and avoid undefined target errors.

velox_common_base
velox_core
velox_dwio_common_exception
Expand Down
63 changes: 61 additions & 2 deletions presto-native-execution/presto_cpp/main/connectors/PrestoToVeloxConnector.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,15 @@
#include "presto_cpp/main/connectors/PrestoToVeloxConnector.h"
#include "presto_cpp/main/types/PrestoToVeloxExpr.h"
#include "presto_cpp/main/types/TypeParser.h"
#include "presto_cpp/presto_protocol/connector/clp/ClpConnectorProtocol.h"
#include "presto_cpp/presto_protocol/connector/hive/HiveConnectorProtocol.h"
#include "presto_cpp/presto_protocol/connector/iceberg/IcebergConnectorProtocol.h"
#include "presto_cpp/presto_protocol/connector/tpch/TpchConnectorProtocol.h"

#include <velox/type/fbhive/HiveTypeParser.h>
#include "velox/connectors/clp/ClpColumnHandle.h"
#include "velox/connectors/clp/ClpConnectorSplit.h"
#include "velox/connectors/clp/ClpTableHandle.h"
#include "velox/connectors/hive/HiveConnector.h"
#include "velox/connectors/hive/HiveConnectorSplit.h"
#include "velox/connectors/hive/HiveDataSink.h"
Expand Down Expand Up @@ -1412,9 +1416,11 @@ IcebergPrestoToVeloxConnector::toVeloxColumnHandle(
// constructor similar to how Hive Connector is handling for bucketing
velox::type::fbhive::HiveTypeParser hiveTypeParser;
auto type = stringToType(icebergColumn->type, typeParser);
connector::hive::HiveColumnHandle::ColumnParseParameters columnParseParameters;
connector::hive::HiveColumnHandle::ColumnParseParameters
columnParseParameters;
if (type->isDate()) {
columnParseParameters.partitionDateValueFormat = connector::hive::HiveColumnHandle::ColumnParseParameters::kDaysSinceEpoch;
columnParseParameters.partitionDateValueFormat = connector::hive::
HiveColumnHandle::ColumnParseParameters::kDaysSinceEpoch;
}
return std::make_unique<connector::hive::HiveColumnHandle>(
icebergColumn->columnIdentity.name,
Expand Down Expand Up @@ -1548,4 +1554,57 @@ std::unique_ptr<protocol::ConnectorProtocol>
TpchPrestoToVeloxConnector::createConnectorProtocol() const {
return std::make_unique<protocol::tpch::TpchConnectorProtocol>();
}

std::unique_ptr<velox::connector::ConnectorSplit>
ClpPrestoToVeloxConnector::toVeloxSplit(
const protocol::ConnectorId& catalogId,
const protocol::ConnectorSplit* connectorSplit,
const protocol::SplitContext* splitContext) const {
auto clpSplit = dynamic_cast<const protocol::clp::ClpSplit*>(connectorSplit);
VELOX_CHECK_NOT_NULL(
clpSplit, "Unexpected split type {}", connectorSplit->_type);
return std::make_unique<connector::clp::ClpConnectorSplit>(
catalogId, clpSplit->path);
}

std::unique_ptr<velox::connector::ColumnHandle>
ClpPrestoToVeloxConnector::toVeloxColumnHandle(
const protocol::ColumnHandle* column,
const TypeParser& typeParser) const {
auto clpColumn = dynamic_cast<const protocol::clp::ClpColumnHandle*>(column);
VELOX_CHECK_NOT_NULL(
clpColumn, "Unexpected column handle type {}", column->_type);
return std::make_unique<connector::clp::ClpColumnHandle>(
clpColumn->columnName,
clpColumn->originalColumnName,
typeParser.parse(clpColumn->columnType),
clpColumn->nullable);
}

std::unique_ptr<velox::connector::ConnectorTableHandle>
ClpPrestoToVeloxConnector::toVeloxTableHandle(
const protocol::TableHandle& tableHandle,
const VeloxExprConverter& exprConverter,
const TypeParser& typeParser,
std::unordered_map<
std::string,
std::shared_ptr<velox::connector::ColumnHandle>>& assignments) const {
auto clpLayout =
std::dynamic_pointer_cast<const protocol::clp::ClpTableLayoutHandle>(
tableHandle.connectorTableLayout);
VELOX_CHECK_NOT_NULL(
clpLayout,
"Unexpected layout type {}",
tableHandle.connectorTableLayout->_type);
return std::make_unique<connector::clp::ClpTableHandle>(
tableHandle.connectorId,
clpLayout->table.schemaTableName.table,
clpLayout->kqlQuery);
}

std::unique_ptr<protocol::ConnectorProtocol>
ClpPrestoToVeloxConnector::createConnectorProtocol() const {
return std::make_unique<protocol::clp::ClpConnectorProtocol>();
}

} // namespace facebook::presto
29 changes: 28 additions & 1 deletion presto-native-execution/presto_cpp/main/connectors/PrestoToVeloxConnector.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,9 @@
*/
#pragma once

#include "presto_cpp/main/types/PrestoToVeloxExpr.h"
#include "presto_cpp/presto_protocol/connector/hive/presto_protocol_hive.h"
#include "presto_cpp/presto_protocol/core/ConnectorProtocol.h"
#include "presto_cpp/main/types/PrestoToVeloxExpr.h"
#include "velox/connectors/Connector.h"
#include "velox/connectors/hive/TableHandle.h"
#include "velox/core/PlanNode.h"
Expand Down Expand Up @@ -223,4 +223,31 @@ class TpchPrestoToVeloxConnector final : public PrestoToVeloxConnector {
std::unique_ptr<protocol::ConnectorProtocol> createConnectorProtocol()
const final;
};

class ClpPrestoToVeloxConnector final : public PrestoToVeloxConnector {
public:
explicit ClpPrestoToVeloxConnector(std::string connectorName)
: PrestoToVeloxConnector(std::move(connectorName)) {}

std::unique_ptr<velox::connector::ConnectorSplit> toVeloxSplit(
const protocol::ConnectorId& catalogId,
const protocol::ConnectorSplit* connectorSplit,
const protocol::SplitContext* splitContext) const final;

std::unique_ptr<velox::connector::ColumnHandle> toVeloxColumnHandle(
const protocol::ColumnHandle* column,
const TypeParser& typeParser) const final;

std::unique_ptr<velox::connector::ConnectorTableHandle> toVeloxTableHandle(
const protocol::TableHandle& tableHandle,
const VeloxExprConverter& exprConverter,
const TypeParser& typeParser,
std::unordered_map<
std::string,
std::shared_ptr<velox::connector::ColumnHandle>>& assignments)
const final;

std::unique_ptr<protocol::ConnectorProtocol> createConnectorProtocol()
const final;
};
} // namespace facebook::presto
9 changes: 9 additions & 0 deletions presto-native-execution/presto_cpp/main/connectors/Registration.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand All @@ -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>());
}

Copy link

@anlowee anlowee Jul 9, 2025

Choose a reason for hiding this comment

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

Need to modify the CMakeLists.txt under this connectors dir:

target_link_libraries(presto_connectors presto_velox_expr_conversion
                      velox_type_fbhive velox_clp_connector)

Copy link

@anlowee anlowee Jul 9, 2025

Choose a reason for hiding this comment

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

otherwise it cannot find the ClpConnectorFactory() when building

Copy link
Author

@wraymo wraymo Jul 9, 2025

Choose a reason for hiding this comment

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

The reason is we haven't updated velox code.

Copy link
Author

@wraymo wraymo Jul 9, 2025

Choose a reason for hiding this comment

The 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)) {
Expand Down Expand Up @@ -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.
Expand Down
1 change: 1 addition & 0 deletions presto-native-execution/presto_cpp/main/tests/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ target_link_libraries(
$<TARGET_OBJECTS:presto_types>
velox_hive_connector
velox_tpch_connector
velox_clp_connector
velox_presto_serializer
velox_functions_prestosql
velox_aggregates
Expand Down
3 changes: 3 additions & 0 deletions presto-native-execution/presto_cpp/main/types/tests/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ target_link_libraries(
velox_dwio_orc_reader
velox_hive_connector
velox_tpch_connector
velox_clp_connector
velox_exec
velox_dwio_common_exception
presto_type_converter
Expand Down Expand Up @@ -64,6 +65,7 @@ target_link_libraries(
velox_functions_lib
velox_hive_connector
velox_tpch_connector
velox_clp_connector
velox_hive_partition_function
velox_presto_serializer
velox_serialization
Expand Down Expand Up @@ -96,6 +98,7 @@ target_link_libraries(
velox_dwio_common
velox_hive_connector
velox_tpch_connector
velox_clp_connector
GTest::gtest
GTest::gtest_main)

Expand Down
2 changes: 2 additions & 0 deletions presto-native-execution/presto_cpp/main/types/tests/PrestoToVeloxConnectorTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ TEST_F(PrestoToVeloxConnectorTest, registerVariousConnectors) {
"iceberg", std::make_unique<IcebergPrestoToVeloxConnector>("iceberg")));
connectorList.emplace_back(
std::pair("tpch", std::make_unique<HivePrestoToVeloxConnector>("tpch")));
connectorList.emplace_back(
std::pair("clp", std::make_unique<ClpPrestoToVeloxConnector>("clp")));

for (auto& [connectorName, connector] : connectorList) {
registerPrestoToVeloxConnector(std::move(connector));
Expand Down
11 changes: 10 additions & 1 deletion presto-native-execution/presto_cpp/presto_protocol/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link

@anlowee anlowee Jul 10, 2025

Choose a reason for hiding this comment

The 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
Copy link

@anlowee anlowee Jul 10, 2025

Choose a reason for hiding this comment

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

Indent seems incorrect, refer to the lines above, they start with a tab not 4 spaces.

Copy link
Author

@wraymo wraymo Jul 10, 2025

Choose a reason for hiding this comment

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

I guess the ide again auto-formats it to spaces. I'll take a took


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
29 changes: 29 additions & 0 deletions presto-native-execution/presto_cpp/presto_protocol/connector/clp/ClpConnectorProtocol.h
View file
Open in desktop
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
Loading
Loading