Skip to content
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
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
6 changes: 6 additions & 0 deletions velox-connector/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
build*/
cmake-build-*/
dist/
*.o
*.so
*.a
101 changes: 101 additions & 0 deletions velox-connector/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
cmake_minimum_required(VERSION 3.28)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Any reasoning for the choice of 3.28?

@20001020ycx 20001020ycx May 21, 2026 •

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Good question. We've always been using this version, and I think it comes from the requirement when building velox: https://github.com/facebookincubator/velox/blob/main/CMakeLists.txt

The Presto worker uses 3.10 and CLP uses anything later than 3.23. So i guess the best way to mitigate is to use what velox has required - 3.28. At lease emprically this version didn't lead to any problem.

Comment thread
20001020ycx marked this conversation as resolved.
Resolved

project(libclp-plugin-velox-connector LANGUAGES CXX)

set(CMAKE_CXX_STANDARD 20)
set(CMAKE_CXX_STANDARD_REQUIRED ON)

# Enable -fPIC for all targets so STATIC libraries can be linked into a shared library.
set(CMAKE_POSITION_INDEPENDENT_CODE ON)

# Match the Presto worker's AVX2 flags to satisfy Folly's F14LinkCheck<Mode> symbol.
#
# NOTE: Including Folly's F14 headers emits a call to F14LinkCheck<Mode>, where Mode is determined
# at compile time by the CPU flags. This plugin includes the Presto worker's headers, which pull in
# Folly's F14 headers transitively. The Presto worker is built with AVX2 (Mode 2); if this plugin is
# built without it, it emits a call to F14LinkCheck<1>, which the Presto worker does not provide,
# causing dlopen to fail with an unresolved symbol error.
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -mavx2")

# ==============================================================================
# Presto/Velox framework headers
# ==============================================================================
include(FetchContent)

message(STATUS "Downloading presto for headers (commit: 8a20b604c8c6348aab671bcd723ea4482c9e3603)")
FetchContent_Declare(
presto_native_execution
GIT_REPOSITORY https://github.com/y-scope/presto.git
GIT_TAG 8a20b604c8c6348aab671bcd723ea4482c9e3603
GIT_SUBMODULES "presto-native-execution/velox"
GIT_SUBMODULES_RECURSE OFF
)
FetchContent_MakeAvailable(presto_native_execution)
set(PRESTO_ROOT ${presto_native_execution_SOURCE_DIR}/presto-native-execution)

# ==============================================================================
# CLP
# ==============================================================================

# CLP options — only build what the plugin needs.
set(CLP_BUILD_CLP_REGEX_UTILS OFF CACHE BOOL "Build CLP regex utils")
set(CLP_BUILD_CLP_S_JSONCONSTRUCTOR OFF CACHE BOOL "Build CLP-S JSON constructor")
set(CLP_BUILD_CLP_S_REDUCER_DEPENDENCIES OFF CACHE BOOL "Build CLP-S reducer dependencies")
set(CLP_BUILD_CLP_S_SEARCH_SQL OFF CACHE BOOL "Build CLP-S search SQL")
set(CLP_BUILD_EXECUTABLES OFF CACHE BOOL "Build CLP executables")
set(CLP_BUILD_TESTING OFF CACHE BOOL "Build CLP tests")

message(STATUS "Downloading CLP (commit: a91e5f71f0715d7d6b3ea7c177e1b39b3e6a24a6)")
FetchContent_Declare(clp
GIT_REPOSITORY https://github.com/y-scope/clp.git
GIT_TAG a91e5f71f0715d7d6b3ea7c177e1b39b3e6a24a6
SOURCE_SUBDIR components/core
)
FetchContent_MakeAvailable(clp)

# ==============================================================================
# Plugin target
# ==============================================================================

# Plugin source tree
add_subdirectory(src)

# Plugin target
add_library(clp-plugin-velox-connector SHARED
src/ClpPluginEntry.cpp
src/ClpPrestoToVeloxConnector.cpp
)

target_include_directories(clp-plugin-velox-connector
PRIVATE
${CMAKE_CURRENT_SOURCE_DIR}/src
${PRESTO_ROOT}
${PRESTO_ROOT}/velox
)

target_link_libraries(clp-plugin-velox-connector
PRIVATE
presto_clp_protocol
presto_clp_connector
)

# Hide all symbols by default to prevent ODR clashes with the Presto worker.
#
# NOTE: The plugin and the Presto worker share overlapping dependencies (e.g., fmt) that may differ
# in version. Without visibility control, the dynamic linker could resolve the plugin's calls to
# the Presto worker's copy of those libraries at dlopen time, causing undefined behavior. The
# following ensure the plugin uses its own copies of each dependency:
#
# - Static linking embeds dependency code into the .so (resolved at link time)
# - CXX_VISIBILITY_PRESET hidden hides those symbols from the dynamic table
# - VISIBILITY_INLINES_HIDDEN hides inline function symbols too
set_target_properties(clp-plugin-velox-connector
PROPERTIES
CXX_VISIBILITY_PRESET hidden
VISIBILITY_INLINES_HIDDEN ON
)

target_link_options(clp-plugin-velox-connector
PRIVATE
-Wl,--version-script=${CMAKE_CURRENT_SOURCE_DIR}/plugin.map
)
6 changes: 6 additions & 0 deletions velox-connector/plugin.map
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
global:
registerExtensions;
local:
*;
};
2 changes: 2 additions & 0 deletions velox-connector/src/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
add_subdirectory(protocol)
add_subdirectory(connector)
28 changes: 28 additions & 0 deletions velox-connector/src/ClpPluginEntry.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
/*
* 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.
*/

#include "ClpPrestoToVeloxConnector.h"
#include "presto_cpp/main/connectors/Registration.h"
#include "connector/ClpConnector.h"

/// Entry point called by presto_server after dlopen().
extern "C" __attribute__((visibility("default"))) void registerExtensions() {
facebook::presto::registerConnectorFactory(
std::make_shared<facebook::velox::connector::clp::ClpConnectorFactory>());

facebook::presto::registerPrestoToVeloxConnector(
std::make_unique<facebook::presto::ClpPrestoToVeloxConnector>(
facebook::velox::connector::clp::ClpConnectorFactory::
kClpConnectorName));
}
75 changes: 75 additions & 0 deletions velox-connector/src/ClpPrestoToVeloxConnector.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
/*
* 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.
*/

#include "ClpPrestoToVeloxConnector.h"
#include "presto_cpp/main/types/TypeParser.h"
#include "connector/ClpColumnHandle.h"
#include "connector/ClpConnectorSplit.h"
#include "connector/ClpTableHandle.h"
#include "velox/common/encode/Base64.h"

namespace facebook::presto {

using namespace velox;

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);
Comment on lines +31 to +33

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Guard the incoming handles before using _type in the error path.

If connectorSplit, column, or tableHandle.connectorTableLayout is null, these checks will dereference ->_type while already handling the failed cast, so the worker crashes before surfacing the intended validation error.

Proposed fix
+  VELOX_CHECK_NOT_NULL(connectorSplit, "Connector split must not be null");
   auto clpSplit = dynamic_cast<const protocol::clp::ClpSplit*>(connectorSplit);
   VELOX_CHECK_NOT_NULL(
       clpSplit, "Unexpected split type {}", connectorSplit->_type);
+  VELOX_CHECK_NOT_NULL(column, "Column handle must not be null");
   auto clpColumn = dynamic_cast<const protocol::clp::ClpColumnHandle*>(column);
   VELOX_CHECK_NOT_NULL(
       clpColumn, "Unexpected column handle type {}", column->_type);
+  VELOX_CHECK_NOT_NULL(
+      tableHandle.connectorTableLayout,
+      "Connector table layout must not be null");
   auto clpLayout =
       std::dynamic_pointer_cast<const protocol::clp::ClpTableLayoutHandle>(
           tableHandle.connectorTableLayout);
   VELOX_CHECK_NOT_NULL(
       clpLayout,
       "Unexpected layout type {}",
       tableHandle.connectorTableLayout->_type);

Also applies to: 45-47, 59-65

🧰 Tools
🪛 Cppcheck (2.20.0)

[style] 32-32: The function 'tableName' is never used.

(unusedFunction)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@velox-connector/src/ClpPrestoToVeloxConnector.cpp` around lines 31 - 33, The
cast and subsequent error message access connectorSplit->_type without first
ensuring connectorSplit (and other handles like column and
tableHandle.connectorTableLayout) are non-null; change the checks around the
dynamic_cast of connectorSplit (the clpSplit variable) so you validate
connectorSplit is not null before referencing connectorSplit->_type in the
VELOX_CHECK_NOT_NULL call, and similarly guard any uses of column and
tableHandle.connectorTableLayout in the other occurrences (the checks around
lines 45-47 and 59-65) so the error path does not dereference ->_type on a null
pointer; update the VELOX_CHECK_NOT_NULL calls or add explicit null checks to
include safe, non-dereferencing messages when the incoming handle is null.

return std::make_unique<connector::clp::ClpConnectorSplit>(
catalogId,
clpSplit->path,
static_cast<int>(clpSplit->type),
clpSplit->kqlQuery);
}

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));
}

std::unique_ptr<velox::connector::ConnectorTableHandle>
ClpPrestoToVeloxConnector::toVeloxTableHandle(
const protocol::TableHandle& tableHandle,
const VeloxExprConverter& /*exprConverter*/,
const TypeParser& /*typeParser*/) 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);
}

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

} // namespace facebook::presto
45 changes: 45 additions & 0 deletions velox-connector/src/ClpPrestoToVeloxConnector.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
/*
* 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/main/connectors/PrestoToVeloxConnector.h"
#include "protocol/ClpConnectorProtocol.h"
#include "presto_cpp/presto_protocol/core/ConnectorProtocol.h"

namespace facebook::presto {

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) const final;

std::unique_ptr<protocol::ConnectorProtocol> createConnectorProtocol()
const final;
};

} // namespace facebook::presto
22 changes: 22 additions & 0 deletions velox-connector/src/connector/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
add_subdirectory(search_lib)

add_library(presto_clp_connector STATIC
ClpConfig.cpp
ClpConnector.cpp
ClpDataSource.cpp
ClpTableHandle.cpp
)
Comment thread
20001020ycx marked this conversation as resolved.
Resolved

target_include_directories(presto_clp_connector
PUBLIC
${CMAKE_CURRENT_SOURCE_DIR}/..
${PRESTO_ROOT}
${PRESTO_ROOT}/velox
)

target_link_libraries(presto_clp_connector
PUBLIC
clp_connector_search
PRIVATE
clp_s::search
)
57 changes: 57 additions & 0 deletions velox-connector/src/connector/ClpColumnHandle.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
/*
* Copyright (c) Facebook, Inc. and its affiliates.
*
* 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 "velox/connectors/Connector.h"

namespace facebook::velox::connector::clp {

class ClpColumnHandle : public ColumnHandle {
public:
ClpColumnHandle(
const std::string& columnName,
const std::string& originalColumnName,
const TypePtr& columnType)
: columnName_(columnName),
originalColumnName_(originalColumnName),
columnType_(columnType) {}

const std::string& name() const override {
return columnName_;
}

const std::string& columnName() const {
return columnName_;
}

const std::string& originalColumnName() const {
return originalColumnName_;
}

const TypePtr& columnType() const {
return columnType_;
}

inline static const std::string jsonStringColumnName_ = "__json_string";

private:
const std::string columnName_;
const std::string originalColumnName_;
const TypePtr columnType_;
};

} // namespace facebook::velox::connector::clp
Loading