From 15205c4be4a310790cb36a5c210901007c34aa0e Mon Sep 17 00:00:00 2001 From: Christian Zentgraf Date: Tue, 19 Nov 2024 17:51:06 -0500 Subject: [PATCH] feat(build): Switch to use C++20 standard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes as a result: - fix warnings with deprecated usage of volatile variables - replace with std::atomic - replace usage of deprecated std::is_pod_v - fix deprecated usage of implicit capture of ‘this’ via ‘[=]’ - fix ambiguity when calling == operator for LambdaTypedExpr - rearrange some link libraries order to prevent undefined symbols - handle AppleClang versions overloading operator<< for sys_seconds input - handle AppleClang format_to function ambiguity between fmt and system headers - remove std=c++17 from the setup helper script which influences the build - disable folly coroutine headers because the library is not built to support it --- CMakeLists.txt | 28 ++++++++++++++++++- README.md | 26 +++++++++++++++++ pyvelox/pyvelox.cpp | 2 +- scripts/setup-helper-functions.sh | 16 +++++------ velox/common/base/CMakeLists.txt | 7 ++++- velox/common/base/Semaphore.h | 4 +-- velox/common/base/tests/ExceptionTest.cpp | 2 +- velox/common/base/tests/Memcpy.cpp | 2 +- velox/connectors/hive/iceberg/IcebergSplit.h | 3 +- velox/core/Expressions.h | 6 +++- velox/duckdb/conversion/tests/CMakeLists.txt | 1 + velox/dwio/common/SelectiveColumnReader.h | 2 +- .../dwio/parquet/tests/writer/CMakeLists.txt | 2 ++ velox/exec/tests/CMakeLists.txt | 2 +- velox/expression/tests/ExprEncodingsTest.cpp | 2 +- velox/expression/tests/ExprTest.cpp | 2 +- velox/external/date/date.h | 11 ++++++++ .../prestosql/aggregates/CMakeLists.txt | 1 + velox/tool/trace/OperatorReplayerBase.cpp | 3 +- velox/vector/BaseVector.h | 2 +- velox/vector/tests/utils/VectorMaker.h | 2 +- velox/vector/tests/utils/VectorTestBase.h | 4 +-- 22 files changed, 103 insertions(+), 27 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 99a791e56b21f..2c7695848a240 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -294,7 +294,21 @@ endif() # Required so velox code can be used in a dynamic library set(CMAKE_POSITION_INDEPENDENT_CODE TRUE) -set(CMAKE_CXX_STANDARD 17) +# For C++20 support we need GNU GCC11 (or later versions) or Clang/AppleClang 15 +# (or later versions) to get support for the used features. +if(NOT + (("${CMAKE_CXX_COMPILER_ID}" STREQUAL "GNU" AND ${CMAKE_CXX_COMPILER_VERSION} + VERSION_GREATER_EQUAL 11) + OR (("${CMAKE_CXX_COMPILER_ID}" STREQUAL "Clang" + OR "${CMAKE_CXX_COMPILER_ID}" STREQUAL "AppleClang") + AND CMAKE_CXX_COMPILER_VERSION VERSION_GREATER_EQUAL 15))) + message( + FATAL_ERROR + "Unsupported compiler ${CMAKE_CXX_COMPILER_ID} with version ${CMAKE_CXX_COMPILER_VERSION} found." + ) +endif() + +set(CMAKE_CXX_STANDARD 20) set(CMAKE_CXX_STANDARD_REQUIRED True) execute_process( @@ -350,6 +364,13 @@ if(ENABLE_ALL_WARNINGS) -Wno-unused-result \ -Wno-format-overflow \ -Wno-strict-aliasing") + # Avoid compiler bug for GCC 12.2.1 + # https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105329 + if(CMAKE_CXX_COMPILER_VERSION VERSION_EQUAL "12.2.1") + set(KNOWN_COMPILER_SPECIFIC_WARNINGS + "${KNOWN_COMPILER_SPECIFIC_WARNINGS} \ + -Wno-restrict") + endif() endif() set(KNOWN_WARNINGS @@ -362,6 +383,11 @@ if(ENABLE_ALL_WARNINGS) set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -Wextra ${KNOWN_WARNINGS}") endif() +# Folly is not built with coroutine support (for now) so avoid using the +# coroutine headers as they pull in symbols that are not found in the folly +# libraries. These will be removed once we build folly with coroutine support. +set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -DFOLLY_CFG_NO_COROUTINES") + message("FINAL CMAKE_CXX_FLAGS=${CMAKE_CXX_FLAGS}") if(${VELOX_ENABLE_GPU}) diff --git a/README.md b/README.md index 7173e6d85579d..f563c6371af7a 100644 --- a/README.md +++ b/README.md @@ -112,6 +112,32 @@ Details on the dependencies and how Velox manages some of them for you Velox also provides the following scripts to help developers setup and install Velox dependencies for a given platform. +### Supported OS and compiler matrix + +The minimum versions of supported compilers: + +| OS | Compiler | Version | +|----|----------|---------| +| Linux | gcc | 11 | +| Linux | clang | 15 | +| macOS | clang | 15 | + +The recommended OS versions and compilers: + +| OS | Compiler | Version | +|----|----------|---------| +| CentOS 9/RHEL 9 | gcc | 12 | +| Ubuntu 22.04 | gcc | 11 | +| macOS | clang | 16 | + +Alternative combinations: + +| OS | Compiler | Version | +|----|----------|---------| +| CentOS 9/RHEL 9 | gcc | 11 | +| Ubuntu 20.04 | gcc | 11 | +| Ubuntu 24.04 | clang | 15 | + ### Setting up dependencies The following setup scripts use the `DEPENDENCY_DIR` environment variable to set the diff --git a/pyvelox/pyvelox.cpp b/pyvelox/pyvelox.cpp index 8f39fb4b904ef..fbcc0c4510351 100644 --- a/pyvelox/pyvelox.cpp +++ b/pyvelox/pyvelox.cpp @@ -282,7 +282,7 @@ static void addExpressionBindings( std::vector inputs; names.reserve(name_input_map.size()); inputs.reserve(name_input_map.size()); - for (const std::pair& pair : + for (const std::pair& pair : name_input_map) { names.push_back(pair.first); inputs.push_back(pair.second); diff --git a/scripts/setup-helper-functions.sh b/scripts/setup-helper-functions.sh index 43010223a5ff4..c477783bb9a01 100755 --- a/scripts/setup-helper-functions.sh +++ b/scripts/setup-helper-functions.sh @@ -111,15 +111,15 @@ function get_cxx_flags { case $CPU_ARCH in "arm64") - echo -n "-mcpu=apple-m1+crc -std=c++17 -fvisibility=hidden" + echo -n "-mcpu=apple-m1+crc -fvisibility=hidden" ;; "avx") - echo -n "-mavx2 -mfma -mavx -mf16c -mlzcnt -std=c++17 -mbmi2" + echo -n "-mavx2 -mfma -mavx -mf16c -mlzcnt -mbmi2" ;; "sse") - echo -n "-msse4.2 -std=c++17" + echo -n "-msse4.2" ;; "aarch64") @@ -138,16 +138,16 @@ function get_cxx_flags { ARM_CPU_PRODUCT=${hex_ARM_CPU_DETECT: -4:3} if [ "$ARM_CPU_PRODUCT" = "$Neoverse_N1" ]; then - echo -n "-mcpu=neoverse-n1 -std=c++17" + echo -n "-mcpu=neoverse-n1" elif [ "$ARM_CPU_PRODUCT" = "$Neoverse_N2" ]; then - echo -n "-mcpu=neoverse-n2 -std=c++17" + echo -n "-mcpu=neoverse-n2" elif [ "$ARM_CPU_PRODUCT" = "$Neoverse_V1" ]; then - echo -n "-mcpu=neoverse-v1 -std=c++17" + echo -n "-mcpu=neoverse-v1" else - echo -n "-march=armv8-a+crc+crypto -std=c++17" + echo -n "-march=armv8-a+crc+crypto" fi else - echo -n "-std=c++17" + echo -n "" fi ;; *) diff --git a/velox/common/base/CMakeLists.txt b/velox/common/base/CMakeLists.txt index cfbfdc90ea669..8e722fd1890f7 100644 --- a/velox/common/base/CMakeLists.txt +++ b/velox/common/base/CMakeLists.txt @@ -42,7 +42,12 @@ velox_add_library( velox_link_libraries( velox_common_base PUBLIC velox_exception Folly::folly fmt::fmt xsimd - PRIVATE velox_common_compression velox_process velox_test_util glog::glog) + PRIVATE + velox_caching + velox_common_compression + velox_process + velox_test_util + glog::glog) if(${VELOX_BUILD_TESTING}) add_subdirectory(tests) diff --git a/velox/common/base/Semaphore.h b/velox/common/base/Semaphore.h index 760a072627359..b52e574e458f2 100644 --- a/velox/common/base/Semaphore.h +++ b/velox/common/base/Semaphore.h @@ -63,8 +63,8 @@ class Semaphore { private: std::mutex mutex_; std::condition_variable cv_; - volatile int32_t count_; - volatile int32_t numWaiting_{0}; + std::atomic count_; + std::atomic numWaiting_{0}; }; } // namespace facebook::velox diff --git a/velox/common/base/tests/ExceptionTest.cpp b/velox/common/base/tests/ExceptionTest.cpp index 9386b8cb672e0..d057eb2dae36a 100644 --- a/velox/common/base/tests/ExceptionTest.cpp +++ b/velox/common/base/tests/ExceptionTest.cpp @@ -41,7 +41,7 @@ struct fmt::formatter { template auto format(const Counter& c, FormatContext& ctx) const { auto x = c.counter++; - return format_to(ctx.out(), "{}", x); + return fmt::format_to(ctx.out(), "{}", x); } }; diff --git a/velox/common/base/tests/Memcpy.cpp b/velox/common/base/tests/Memcpy.cpp index 2ab51f87815e1..4d7aeca1c1905 100644 --- a/velox/common/base/tests/Memcpy.cpp +++ b/velox/common/base/tests/Memcpy.cpp @@ -74,7 +74,7 @@ int main(int argc, char** argv) { Semaphore sem(0); std::vector ops; ops.resize(FLAGS_threads); - volatile uint64_t totalSum = 0; + std::atomic totalSum = 0; uint64_t totalUsec = 0; for (auto repeat = 0; repeat < FLAGS_repeats; ++repeat) { // Read once through 'other' to clear cache effects. diff --git a/velox/connectors/hive/iceberg/IcebergSplit.h b/velox/connectors/hive/iceberg/IcebergSplit.h index 6c1f4cc6bc3f3..f05d28103b06d 100644 --- a/velox/connectors/hive/iceberg/IcebergSplit.h +++ b/velox/connectors/hive/iceberg/IcebergSplit.h @@ -18,11 +18,10 @@ #include #include "velox/connectors/hive/HiveConnectorSplit.h" +#include "velox/connectors/hive/iceberg/IcebergDeleteFile.h" namespace facebook::velox::connector::hive::iceberg { -struct IcebergDeleteFile; - struct HiveIcebergSplit : public connector::hive::HiveConnectorSplit { std::vector deleteFiles; diff --git a/velox/core/Expressions.h b/velox/core/Expressions.h index a1f9502e559fb..9c7f4b714e773 100644 --- a/velox/core/Expressions.h +++ b/velox/core/Expressions.h @@ -565,7 +565,11 @@ class LambdaTypedExpr : public ITypedExpr { if (*casted->type() != *this->type()) { return false; } - return *signature_ == *casted->signature_ && *body_ == *casted->body_; + return operator==(*casted); + } + + bool operator==(const LambdaTypedExpr& other) const { + return *signature_ == *other.signature_ && *body_ == *other.body_; } folly::dynamic serialize() const override; diff --git a/velox/duckdb/conversion/tests/CMakeLists.txt b/velox/duckdb/conversion/tests/CMakeLists.txt index 3a4607dd8a898..6918919ecf0f9 100644 --- a/velox/duckdb/conversion/tests/CMakeLists.txt +++ b/velox/duckdb/conversion/tests/CMakeLists.txt @@ -20,6 +20,7 @@ add_test(velox_duckdb_conversion_test velox_duckdb_conversion_test) target_link_libraries( velox_duckdb_conversion_test velox_duckdb_parser + velox_common_base velox_parse_expression velox_functions_prestosql velox_functions_lib diff --git a/velox/dwio/common/SelectiveColumnReader.h b/velox/dwio/common/SelectiveColumnReader.h index c4b6acfe20a2b..e543a3032b29e 100644 --- a/velox/dwio/common/SelectiveColumnReader.h +++ b/velox/dwio/common/SelectiveColumnReader.h @@ -335,7 +335,7 @@ class SelectiveColumnReader { template inline void addValue(T value) { static_assert( - std::is_pod_v, + std::is_standard_layout_v, "General case of addValue is only for primitive types"); VELOX_DCHECK_NOT_NULL(rawValues_); VELOX_DCHECK_LE((numValues_ + 1) * sizeof(T), values_->capacity()); diff --git a/velox/dwio/parquet/tests/writer/CMakeLists.txt b/velox/dwio/parquet/tests/writer/CMakeLists.txt index 01e718dc22c46..4cdcc3ce851d9 100644 --- a/velox/dwio/parquet/tests/writer/CMakeLists.txt +++ b/velox/dwio/parquet/tests/writer/CMakeLists.txt @@ -24,6 +24,7 @@ target_link_libraries( velox_dwio_parquet_writer velox_dwio_common_test_utils velox_vector_fuzzer + velox_caching Boost::regex velox_link_libs Folly::folly @@ -42,6 +43,7 @@ target_link_libraries( velox_parquet_writer_test velox_dwio_parquet_writer velox_dwio_common_test_utils + velox_caching velox_link_libs Boost::regex Folly::folly diff --git a/velox/exec/tests/CMakeLists.txt b/velox/exec/tests/CMakeLists.txt index a7b1a0996e036..0889f297d6bc7 100644 --- a/velox/exec/tests/CMakeLists.txt +++ b/velox/exec/tests/CMakeLists.txt @@ -238,8 +238,8 @@ add_executable(velox_cache_fuzzer_test CacheFuzzerTest.cpp) target_link_libraries( velox_cache_fuzzer_test - velox_cache_fuzzer velox_fuzzer_util + velox_cache_fuzzer GTest::gtest GTest::gtest_main) diff --git a/velox/expression/tests/ExprEncodingsTest.cpp b/velox/expression/tests/ExprEncodingsTest.cpp index 0cb706072ee14..51eb52628bd01 100644 --- a/velox/expression/tests/ExprEncodingsTest.cpp +++ b/velox/expression/tests/ExprEncodingsTest.cpp @@ -490,7 +490,7 @@ class ExprEncodingsTest execCtx_->pool(), vector->type(), vector->size(), - std::make_unique([=](RowSet /*rows*/) { + std::make_unique([=, this](RowSet /*rows*/) { auto indices = makeIndices(vector->size(), [](auto row) { return row; }); return wrapInDictionary(indices, vector->size(), vector); diff --git a/velox/expression/tests/ExprTest.cpp b/velox/expression/tests/ExprTest.cpp index 507c75853f5d6..c39a19f285e01 100644 --- a/velox/expression/tests/ExprTest.cpp +++ b/velox/expression/tests/ExprTest.cpp @@ -3958,7 +3958,7 @@ TEST_P(ParameterizedExprTest, cseOverLazyDictionary) { pool(), BIGINT(), 5, - std::make_unique([=](RowSet /*rows*/) { + std::make_unique([=, this](RowSet /*rows*/) { return wrapInDictionary( makeIndicesInReverse(5), makeFlatVector({8, 9, 10, 11, 12})); diff --git a/velox/external/date/date.h b/velox/external/date/date.h index ac6d636dd4283..f275105ea1af0 100644 --- a/velox/external/date/date.h +++ b/velox/external/date/date.h @@ -3970,6 +3970,16 @@ make_time(const std::chrono::duration& d) return hh_mm_ss>(d); } +/// Building on macOS using Apple Clang 15.0.0.15000309 (Xcode_15.4) results in +/// ambiguous symbols (related to std::chrono::duration type parameter) because +/// the newer SDK includes a definiton for this overloaded operator when C++20 standard is used. +/// AppleClang 15.0.0.15000309 (Xcode_15.4) or newer does not need the overloaded definition but +/// AppleClang 15.0.0.15000100 (Xcode_15.2), for example, does. +/// With the introduction of AppleClang 1600.0.26.3 the behavior reverts back to the 15.0.0.15000100 +/// or earlier behavior and the function requires the overload again. +/// This check is for allowing multiple versions AppleClang to build Velox. +#if !defined(__APPLE__) || \ + (defined(__APPLE__) && ((__apple_build_version__ < 15000309) || (__apple_build_version__ >= 16000026))) template inline typename std::enable_if @@ -3983,6 +3993,7 @@ operator<<(std::basic_ostream& os, const sys_time& tp) auto const dp = date::floor(tp); return os << year_month_day(dp) << ' ' << make_time(tp-dp); } +#endif // !defined(__APPLE__) || (defined(__APPLE__) && (__apple_build_version__ < 15000309)) template inline diff --git a/velox/functions/prestosql/aggregates/CMakeLists.txt b/velox/functions/prestosql/aggregates/CMakeLists.txt index 986de438474ef..38691c9071ecb 100644 --- a/velox/functions/prestosql/aggregates/CMakeLists.txt +++ b/velox/functions/prestosql/aggregates/CMakeLists.txt @@ -53,6 +53,7 @@ velox_link_libraries( velox_exec velox_expression velox_presto_serializer + velox_presto_types velox_functions_aggregates velox_functions_lib velox_functions_util diff --git a/velox/tool/trace/OperatorReplayerBase.cpp b/velox/tool/trace/OperatorReplayerBase.cpp index e73ed36ca572c..8c95792d107dc 100644 --- a/velox/tool/trace/OperatorReplayerBase.cpp +++ b/velox/tool/trace/OperatorReplayerBase.cpp @@ -94,7 +94,8 @@ core::PlanNodePtr OperatorReplayerBase::createPlan() const { std::function OperatorReplayerBase::replayNodeFactory(const core::PlanNode* node) const { - return [=](const core::PlanNodeId& nodeId, + return [=, this]( + const core::PlanNodeId& nodeId, const core::PlanNodePtr& source) -> core::PlanNodePtr { return createPlanNode(node, nodeId, source); }; diff --git a/velox/vector/BaseVector.h b/velox/vector/BaseVector.h index 23b9dc7b8f011..2a9fffe58d58c 100644 --- a/velox/vector/BaseVector.h +++ b/velox/vector/BaseVector.h @@ -1061,7 +1061,7 @@ struct fmt::formatter { auto format( const facebook::velox::VectorEncoding::Simple& x, FormatContext& ctx) const { - return format_to( + return fmt::format_to( ctx.out(), "{}", facebook::velox::VectorEncoding::mapSimpleToName(x)); } }; diff --git a/velox/vector/tests/utils/VectorMaker.h b/velox/vector/tests/utils/VectorMaker.h index a047930dd035c..e8ee2de262ce9 100644 --- a/velox/vector/tests/utils/VectorMaker.h +++ b/velox/vector/tests/utils/VectorMaker.h @@ -145,7 +145,7 @@ class VectorMaker { pool_, CppToType::create(), size, - std::make_unique([=](RowSet rowSet) { + std::make_unique([=, this](RowSet rowSet) { // Populate requested rows with correct data and fill in gaps with // "garbage". SelectivityVector rows(rowSet.back() + 1, false); diff --git a/velox/vector/tests/utils/VectorTestBase.h b/velox/vector/tests/utils/VectorTestBase.h index b0a363527d9ca..50e23149acae5 100644 --- a/velox/vector/tests/utils/VectorTestBase.h +++ b/velox/vector/tests/utils/VectorTestBase.h @@ -781,7 +781,7 @@ class VectorTestBase { pool(), CppToType::create(), size, - std::make_unique([=](RowSet rows) { + std::make_unique([=, this](RowSet rows) { if (expectedSize.has_value()) { VELOX_CHECK_EQ(rows.size(), *expectedSize); } @@ -799,7 +799,7 @@ class VectorTestBase { pool(), vector->type(), vector->size(), - std::make_unique([=](RowSet /*rows*/) { + std::make_unique([=, this](RowSet /*rows*/) { auto indices = makeIndices(vector->size(), [](auto row) { return row; }); return wrapInDictionary(indices, vector->size(), vector);