From 33f0a8f4f408c274ad41058e8d99ecf877ee86e7 Mon Sep 17 00:00:00 2001 From: Christian Zentgraf Date: Thu, 28 Nov 2024 19:58:56 -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 - turn on warning suppression for 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 If the implicit capture of this is fixed it causes upstream problems by issuing warning c++20-extensions as upstream users are not yet on C++20. Thus, adding to suppress the warning -Wdeprecated-this-capture for now. --- CMakeLists.txt | 29 ++++++++++++++++++- README.md | 26 +++++++++++++++++ pyvelox/pyvelox.cpp | 2 +- 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/external/date/date.h | 11 +++++++ .../prestosql/aggregates/CMakeLists.txt | 1 + velox/vector/BaseVector.h | 2 +- 16 files changed, 89 insertions(+), 13 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 9de9f5a08e48..c031fd12496e 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -311,7 +311,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) set(CMAKE_CXX_EXTENSIONS ON) # Big Int is an extension @@ -368,6 +382,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 @@ -375,11 +396,17 @@ if(ENABLE_ALL_WARNINGS) -Wno-unused-parameter \ -Wno-sign-compare \ -Wno-ignored-qualifiers \ + -Wno-deprecated-this-capture \ ${KNOWN_COMPILER_SPECIFIC_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 7173e6d85579..f563c6371af7 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 8f39fb4b904e..fbcc0c451035 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/velox/common/base/CMakeLists.txt b/velox/common/base/CMakeLists.txt index dad8128a343f..375b856d54e3 100644 --- a/velox/common/base/CMakeLists.txt +++ b/velox/common/base/CMakeLists.txt @@ -43,7 +43,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 760a07262735..b52e574e458f 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 9386b8cb672e..d057eb2dae36 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 2ab51f87815e..4d7aeca1c190 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 6c1f4cc6bc3f..f05d28103b06 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 a708f0060a41..6753b5129aa2 100644 --- a/velox/core/Expressions.h +++ b/velox/core/Expressions.h @@ -591,7 +591,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 3a4607dd8a89..6918919ecf0f 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 c4b6acfe20a2..e543a3032b29 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 01e718dc22c4..4cdcc3ce851d 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 2190fc5ae567..bf11503e2741 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/external/date/date.h b/velox/external/date/date.h index ac6d636dd428..a906e404a818 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) || (__apple_build_version__ >= 16000026))) template inline diff --git a/velox/functions/prestosql/aggregates/CMakeLists.txt b/velox/functions/prestosql/aggregates/CMakeLists.txt index 986de438474e..38691c9071ec 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/vector/BaseVector.h b/velox/vector/BaseVector.h index 883ee403cfb2..2ec0789a9a2d 100644 --- a/velox/vector/BaseVector.h +++ b/velox/vector/BaseVector.h @@ -1068,7 +1068,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)); } };