Skip to content
Merged
Show file tree
Hide file tree
Changes from 10 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
4 changes: 3 additions & 1 deletion presto-native-execution/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,9 @@ set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${SCRIPT_CXX_FLAGS}")

# Known warnings that are benign can be disabled.
set(DISABLED_WARNINGS
"-Wno-nullability-completeness -Wno-deprecated-declarations")
"-Wno-nullability-completeness \
-Wno-deprecated-declarations \
-Wno-restrict")
Comment on lines +36 to +38
Copy link

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

Verify the necessity of suppressing the -Wrestrict warning.

The addition of -Wno-restrict to the disabled warnings list suggests that the new CLP connector code triggers this warning. Please verify that this warning suppression is necessary and doesn't hide important issues.


🏁 Script executed:

#!/bin/bash
# Description: Check if the CLP connector code triggers -Wrestrict warnings
# Expected: Find instances where this warning would be triggered

# Search for patterns that commonly trigger -Wrestrict warnings in CLP-related files
echo "Searching for potential -Wrestrict triggers in CLP connector code..."
rg -A 3 -B 3 "strcpy|strncpy|sprintf|memcpy|memmove" --type cpp --glob "*clp*" || true
rg -A 3 -B 3 "restrict|__restrict" --type cpp --glob "*clp*" || true

Length of output: 304


Remove unnecessary -Wno-restrict suppression

No usage patterns (e.g. strcpy, memcpy, or pointer qualifiers like restrict) were found in the CLP connector code that would trigger a -Wrestrict warning. To avoid hiding potential issues, please remove the -Wno-restrict entry.

• File to update: presto-native-execution/CMakeLists.txt (around lines 34–36)
• Suggested diff:

-    "-Wno-nullability-completeness \
-     -Wno-deprecated-declarations \
-     -Wno-restrict")
+    "-Wno-nullability-completeness \
+     -Wno-deprecated-declarations")
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
"-Wno-nullability-completeness \
-Wno-deprecated-declarations \
-Wno-restrict")
"-Wno-nullability-completeness \
-Wno-deprecated-declarations")
🤖 Prompt for AI Agents
In presto-native-execution/CMakeLists.txt around lines 34 to 36, remove the
compiler flag '-Wno-restrict' from the list of warning suppressions since no
code in the CLP connector triggers this warning. This will prevent hiding
potential issues related to restrict usage.


# Important warnings that must be explicitly enabled.
set(ENABLE_WARNINGS "-Wreorder")
Expand Down
1 change: 1 addition & 0 deletions presto-native-execution/presto_cpp/main/CMakeLists.txt
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
velox_common_base
velox_core
velox_dwio_common_exception
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,4 +18,4 @@ if(PRESTO_ENABLE_ARROW_FLIGHT_CONNECTOR)
endif()

target_link_libraries(presto_connectors presto_velox_expr_conversion
velox_type_fbhive)
velox_type_fbhive velox_clp_connector)
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need this? If so, let's alphabetize the list of libraries to link against.

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

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

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Same optional-build issue for test target

presto_server_test now always links against velox_clp_connector. Add the same if(PRESTO_ENABLE_CLP_CONNECTOR) guard to avoid link failures when CLP isn’t present.

🤖 Prompt for AI Agents
In presto-native-execution/presto_cpp/main/tests/CMakeLists.txt around lines 48
to 52, the test target presto_server_test always links against
velox_clp_connector, which causes link failures if the CLP connector is not
enabled. Wrap the addition of velox_clp_connector in an
if(PRESTO_ENABLE_CLP_CONNECTOR) conditional block to ensure it is only linked
when the CLP connector is present, preventing build errors.

velox_aggregates
Expand Down
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
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Guard CLP linkage in presto_velox_split_test

Add a feature flag guard around velox_clp_connector just like the main connector target.

🤖 Prompt for AI Agents
In presto-native-execution/presto_cpp/main/types/tests/CMakeLists.txt around
lines 28 to 32, the dependency on velox_clp_connector in the
presto_velox_split_test target is not guarded by a feature flag. Add a
conditional check for the CLP feature flag around the inclusion of
velox_clp_connector, similar to how it is done in the main connector target, to
ensure the linkage only occurs when the feature is enabled.

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

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Guard CLP linkage in presto_expressions_test

Same optional-build concern as above.

🤖 Prompt for AI Agents
In presto-native-execution/presto_cpp/main/types/tests/CMakeLists.txt around
lines 66 to 70, the linkage of the velox_clp_connector in the
presto_expressions_test target is not guarded for optional builds. Modify the
CMakeLists.txt to conditionally link velox_clp_connector only if the CLP
component is enabled, using appropriate CMake conditional statements to prevent
build errors when CLP is not included.

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

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Guard CLP linkage in presto_to_velox_connector_test

Mirror the conditional linkage pattern so builds that exclude CLP stay healthy.

🤖 Prompt for AI Agents
In presto-native-execution/presto_cpp/main/types/tests/CMakeLists.txt around
lines 99 to 103, the target_link_libraries for presto_to_velox_connector_test
always includes velox_clp_connector, which can cause build failures if CLP is
excluded. Modify the CMakeLists.txt to conditionally link velox_clp_connector
only if CLP is enabled, mirroring the existing pattern used elsewhere to guard
CLP linkage and ensure builds without CLP remain healthy.


Expand Down
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")));
Copy link
Member

Choose a reason for hiding this comment

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

To alphabetize, can we move this above line 27?


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
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
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
Loading
Loading