Skip to content
Closed
Show file tree
Hide file tree
Changes from 6 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 33 to +36
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

Avoid embedded back-slashes & newline chars in DISABLED_WARNINGS; gate -Wno-restrict on compiler support

The 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-restrict is recognised by GCC but not by Clang, triggering “unknown warning option” errors on macOS CI.

Proposed fix:

-set(DISABLED_WARNINGS
-    "-Wno-nullability-completeness \
-     -Wno-deprecated-declarations \
-     -Wno-restrict")
+set(DISABLED_WARNINGS
+    "-Wno-nullability-completeness"
+    "-Wno-deprecated-declarations"
+    "-Wno-restrict")
+
+# Only keep -Wno-restrict if the compiler understands it.
+include(CheckCXXCompilerFlag)
+check_cxx_compiler_flag("-Wrestrict" COMPILER_SUPPORTS_WRESTRICT)
+if(NOT COMPILER_SUPPORTS_WRESTRICT)
+  list(REMOVE_ITEM DISABLED_WARNINGS "-Wno-restrict")
+endif()

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:

#!/bin/bash
# Show how DISABLED_WARNINGS is defined and used in CMakeLists
rg -n "DISABLED_WARNINGS" -A5 -B2 presto-native-execution/CMakeLists.txt

Length of output: 556


🌐 Web query:

CMake backslash newline line continuation

💡 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:

message("This is the first line. \
This line is continued.")

This outputs:

This is the first line. This line is continued.

Key details:

  • This feature only works within quoted arguments (i.e., text inside "...").
  • The final \ 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]
  • This makes your CMake code more readable when handling long strings, command arguments, or messages.

Note:
Versions of CMake prior to 3.0 do not support this backslash-newline continuation inside quoted arguments.[2]

Summary table:

Scenario Effect
Quoted argument Backslash + newline = line continues, no newline in value[2]
Unquoted context No line continuation; lines are treated as separate commands or arguments[2]

References to the CMake manual confirm this syntax:

The final \ on any line ending in an odd number of backslashes is treated as a line continuation and ignored along with the immediately following newline character.[2]

If you need to make long argument lists or messages in CMake readable, this is the preferred technique.

Citations:


Gate -Wno-restrict on compiler support

File: presto-native-execution/CMakeLists.txt
Lines: 33–36

CMake’s quoted-argument backslash+newline continuation removes the backslashes and newlines, so:

set(DISABLED_WARNINGS
    "-Wno-nullability-completeness \
     -Wno-deprecated-declarations \
     -Wno-restrict")

already produces a single string containing three space-separated flags—nothing is silently dropped.

However, Clang does not recognise -Wno-restrict and will emit unknown-option errors on macOS CI. Please add immediately after line 36:

+# Only keep -Wno-restrict if the compiler recognises it.
+include(CheckCXXCompilerFlag)
+check_cxx_compiler_flag("-Wno-restrict" COMPILER_SUPPORTS_WNO_RESTRICT)
+if(NOT COMPILER_SUPPORTS_WNO_RESTRICT)
+  list(REMOVE_ITEM DISABLED_WARNINGS "-Wno-restrict")
+endif()

Then verify the build on both GCC and Clang before merging.

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In presto-native-execution/CMakeLists.txt around lines 33 to 36, the
-Wno-restrict flag is included unconditionally in the DISABLED_WARNINGS set, but
Clang does not support this flag and will error on macOS CI. To fix this, add a
conditional check immediately after line 36 to detect if the compiler is GCC and
only then append -Wno-restrict to DISABLED_WARNINGS. This ensures compatibility
with both GCC and Clang. After making this change, verify the build works
correctly on both compilers before merging.


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

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

Copy link

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

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

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

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

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

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