feat(build): Switch to use C++20 standard

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

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

pyvelox/pyvelox.cpp

@@ -282,7 +282,7 @@ static void addExpressionBindings(
             std::vector<VectorPtr> inputs;
             names.reserve(name_input_map.size());
             inputs.reserve(name_input_map.size());
-            for (const std::pair<std::string, VectorPtr>& pair :
+            for (const std::pair<const std::string, VectorPtr>& pair :
                  name_input_map) {
               names.push_back(pair.first);
               inputs.push_back(pair.second);

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

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)

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<int32_t> count_;
+  std::atomic<int32_t> numWaiting_{0};
 };
 
 } // namespace facebook::velox

velox/common/base/tests/ExceptionTest.cpp

@@ -41,7 +41,7 @@ struct fmt::formatter<Counter> {
   template <typename FormatContext>
   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);
   }
 };
 

velox/common/base/tests/Memcpy.cpp

@@ -74,7 +74,7 @@ int main(int argc, char** argv) {
   Semaphore sem(0);
   std::vector<CopyCallable> ops;
   ops.resize(FLAGS_threads);
-  volatile uint64_t totalSum = 0;
+  std::atomic<uint64_t> totalSum = 0;
   uint64_t totalUsec = 0;
   for (auto repeat = 0; repeat < FLAGS_repeats; ++repeat) {
     // Read once through 'other' to clear cache effects.

velox/connectors/hive/iceberg/IcebergSplit.h

@@ -18,11 +18,10 @@
 #include <string>
 
 #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<IcebergDeleteFile> deleteFiles;
 

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;

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

velox/dwio/common/SelectiveColumnReader.h

@@ -335,7 +335,7 @@ class SelectiveColumnReader {
   template <typename T>
   inline void addValue(T value) {
     static_assert(
-        std::is_pod_v<T>,
+        std::is_standard_layout_v<T>,
         "General case of addValue is only for primitive types");
     VELOX_DCHECK_NOT_NULL(rawValues_);
     VELOX_DCHECK_LE((numValues_ + 1) * sizeof(T), values_->capacity());

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

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)
 

velox/expression/tests/ExprEncodingsTest.cpp

@@ -490,7 +490,7 @@ class ExprEncodingsTest
         execCtx_->pool(),
         vector->type(),
         vector->size(),
-        std::make_unique<SimpleVectorLoader>([=](RowSet /*rows*/) {
+        std::make_unique<SimpleVectorLoader>([=, this](RowSet /*rows*/) {
           auto indices =
               makeIndices(vector->size(), [](auto row) { return row; });
           return wrapInDictionary(indices, vector->size(), vector);

velox/expression/tests/ExprTest.cpp

@@ -3958,7 +3958,7 @@ TEST_P(ParameterizedExprTest, cseOverLazyDictionary) {
           pool(),
           BIGINT(),
           5,
-          std::make_unique<SimpleVectorLoader>([=](RowSet /*rows*/) {
+          std::make_unique<SimpleVectorLoader>([=, this](RowSet /*rows*/) {
             return wrapInDictionary(
                 makeIndicesInReverse(5),
                 makeFlatVector<int64_t>({8, 9, 10, 11, 12}));

velox/external/date/date.h

@@ -3970,6 +3970,16 @@ make_time(const std::chrono::duration<Rep, Period>& d)
     return hh_mm_ss<std::chrono::duration<Rep, Period>>(d);
 }
 
+/// Building on macOS using Apple Clang 15.0.0.15000309 (Xcode_15.4) results in
+/// ambiguous symbols (related to std::chrono::duration<long long> 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 <class CharT, class Traits, class Duration>
 inline
 typename std::enable_if
@@ -3983,6 +3993,7 @@ operator<<(std::basic_ostream<CharT, Traits>& os, const sys_time<Duration>& tp)
     auto const dp = date::floor<days>(tp);
     return os << year_month_day(dp) << ' ' << make_time(tp-dp);
 }
+#endif // !defined(__APPLE__) || (defined(__APPLE__) && (__apple_build_version__ < 15000309))
 
 template <class CharT, class Traits>
 inline

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

velox/tool/trace/OperatorReplayerBase.cpp

@@ -94,7 +94,8 @@ core::PlanNodePtr OperatorReplayerBase::createPlan() const {
 
 std::function<core::PlanNodePtr(std::string, core::PlanNodePtr)>
 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);
   };

velox/vector/BaseVector.h

@@ -1061,7 +1061,7 @@ struct fmt::formatter<facebook::velox::VectorEncoding::Simple> {
   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));
   }
 };

velox/vector/tests/utils/VectorMaker.h

@@ -145,7 +145,7 @@ class VectorMaker {
         pool_,
         CppToType<T>::create(),
         size,
-        std::make_unique<SimpleVectorLoader>([=](RowSet rowSet) {
+        std::make_unique<SimpleVectorLoader>([=, this](RowSet rowSet) {
           // Populate requested rows with correct data and fill in gaps with
           // "garbage".
           SelectivityVector rows(rowSet.back() + 1, false);

velox/vector/tests/utils/VectorTestBase.h

@@ -781,7 +781,7 @@ class VectorTestBase {
         pool(),
         CppToType<T>::create(),
         size,
-        std::make_unique<SimpleVectorLoader>([=](RowSet rows) {
+        std::make_unique<SimpleVectorLoader>([=, 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<SimpleVectorLoader>([=](RowSet /*rows*/) {
+        std::make_unique<SimpleVectorLoader>([=, this](RowSet /*rows*/) {
           auto indices =
               makeIndices(vector->size(), [](auto row) { return row; });
           return wrapInDictionary(indices, vector->size(), vector);