DO NOT LAND fix: Bug fixes for macOS arm64 unit test failures (reference, do not merge)

CMake/resolve_dependency_modules/s2geometry.cmake

@@ -38,14 +38,30 @@ block()
 
   velox_resolve_dependency_url(S2GEOMETRY)
 
+  set(
+    VELOX_S2GEOMETRY_PATCHES
+    ${CMAKE_CURRENT_LIST_DIR}/s2geometry/s2geometry-gcc12-max.patch
+  )
+  # On Apple platforms folly defines `nallocx` as a null function pointer
+  # because weak symbols cannot be left undefined in Mach-O. That data symbol
+  # wins over s2's weak function definition, so s2's call jumps into __DATA and
+  # crashes with SIGBUS.
+  if(APPLE)
+    list(
+      APPEND
+      VELOX_S2GEOMETRY_PATCHES
+      ${CMAKE_CURRENT_LIST_DIR}/s2geometry/s2geometry-apple-nallocx.patch
+    )
+  endif()
+
   FetchContent_Declare(
     s2geometry
     URL ${VELOX_S2GEOMETRY_SOURCE_URL}
     URL_HASH ${VELOX_S2GEOMETRY_BUILD_SHA256_CHECKSUM}
     OVERRIDE_FIND_PACKAGE
     SYSTEM
     EXCLUDE_FROM_ALL
-    PATCH_COMMAND git apply ${CMAKE_CURRENT_LIST_DIR}/s2geometry/s2geometry-gcc12-max.patch
+    PATCH_COMMAND git apply ${VELOX_S2GEOMETRY_PATCHES}
   )
 
   list(APPEND CMAKE_MODULE_PATH "${s2geometry_SOURCE_DIR}/cmake")

CMake/resolve_dependency_modules/s2geometry/s2geometry-apple-nallocx.patch

@@ -0,0 +1,32 @@
+Avoid the bare `nallocx` symbol on Apple platforms.
+
+s2geometry declares `nallocx` as a function and ships a weak definition that
+simply returns the requested size. On Apple platforms a weak symbol cannot be
+left undefined in Mach-O, so folly builds with FOLLY_HAVE_WEAK_SYMBOLS=0 and
+instead defines `size_t (*nallocx)(size_t, int) = nullptr;` -- a null function
+pointer in the global namespace. When both libraries are linked, s2's call
+binds to folly's data symbol, jumps into __DATA and crashes with SIGBUS.
+
+Using the requested size directly is exactly what s2's own weak implementation
+does, so behaviour is unchanged where the allocator gives no size feedback.
+
+--- a/src/s2/util/gtl/compact_array.h
++++ b/src/s2/util/gtl/compact_array.h
+@@ -390,7 +390,17 @@
+     if (n <= old_capacity) return;
+     size_type new_n = n;
+     if (new_n > kInlined) {
++#if defined(__APPLE__)
++      // Do not reference the global `nallocx` symbol on Apple platforms. folly
++      // defines it as a null function pointer there (FOLLY_HAVE_WEAK_SYMBOLS=0
++      // because Mach-O cannot leave a weak symbol undefined), and that data
++      // symbol wins over s2's weak function, so the call jumps into __DATA and
++      // crashes with SIGBUS. Using the requested size matches s2's own default
++      // weak implementation of nallocx.
++      new_n = n;
++#else
+       new_n = nallocx(n * sizeof(T), 0) / sizeof(T);
++#endif
+     }
+     set_capacity(new_n);
+     if (MayBeInlined()) {

scripts/setup-common.sh

@@ -346,6 +346,13 @@ function install_s2geometry {
     # Apply the same GCC 12 patch used by the BUNDLED CMake resolver.
     patch -p1 -d "${DEPENDENCY_DIR}/s2geometry" < \
       "${SCRIPT_DIR}/../CMake/resolve_dependency_modules/s2geometry/s2geometry-gcc12-max.patch" || true
+    # On macOS folly defines `nallocx` as a null function pointer, which
+    # collides with s2's weak function and crashes with SIGBUS. Apply the same
+    # patch the BUNDLED CMake resolver uses.
+    if [[ $(uname) == "Darwin" ]]; then
+      patch -p1 -d "${DEPENDENCY_DIR}/s2geometry" < \
+        "${SCRIPT_DIR}/../CMake/resolve_dependency_modules/s2geometry/s2geometry-apple-nallocx.patch" || true
+    fi
     cmake_install_dir s2geometry -DBUILD_TESTING=OFF -DBUILD_TESTS=OFF -DBUILD_SHARED_LIBS=OFF
   fi
 }

velox/common/base/tests/SkewedPartitionBalancerTest.cpp

@@ -395,7 +395,9 @@ TEST_F(SkewedPartitionRebalancerTest, singleThreadFuzz) {
           fmt::format("taskCount {}, iteration {}", taskCount, iteration));
       const uint64_t processedBytes = 1 + folly::Random::rand32(512, rng);
       balancer->addProcessedBytes(processedBytes);
-      const auto numPartitons = folly::Random::rand32(32, rng);
+      // rebalance() requires at least one recorded row; adding processed
+      // bytes alone violates the balancer's contract and throws.
+      const auto numPartitons = 1 + folly::Random::rand32(32, rng);
       for (auto i = 0; i < numPartitons; ++i) {
         const auto partition = folly::Random::rand32(32, rng);
         const auto numRows = 1 + folly::Random::rand32(32, rng);

velox/common/caching/StringIdMap.cpp

@@ -36,7 +36,15 @@ void StringIdMap::release(uint64_t id) {
     if (--it->second.numInUse == 0) {
       pinnedSize_ -= it->second.string.size();
       auto strIter = stringToId_.find(it->second.string);
-      VELOX_DCHECK(strIter != stringToId_.end());
+      // Fail loudly instead of erasing an end() iterator. VELOX_DCHECK is
+      // compiled out in release builds, so a broken invariant would otherwise
+      // silently corrupt memory.
+      VELOX_CHECK(
+          strIter != stringToId_.end(),
+          "StringIdMap is inconsistent: id {} maps to string '{}' which is "
+          "missing from the string index",
+          id,
+          it->second.string);
       stringToId_.erase(strIter);
       idToEntry_.erase(it);
     }

velox/common/dynamic_registry/tests/DynamicLinkTest.cpp

@@ -231,12 +231,11 @@ TEST_F(DynamicLinkTest, dynamicLoadFuncNonDefaultRegistry) {
 
   EXPECT_EQ(3, dynamicFunctionNestedCall());
 
-  // Testing missing default registry function name.
+  // Testing missing default registry function name. Everything after the
+  // prefix comes from dlerror() and is worded differently by each platform's
+  // dynamic loader, so only match the part Velox produces.
   VELOX_ASSERT_THROW(
-      loadDynamicLibrary(libraryPath),
-      fmt::format(
-          "Couldn't find Velox registry symbol: {}: undefined symbol: registerExtensions",
-          libraryPath));
+      loadDynamicLibrary(libraryPath), "Couldn't find Velox registry symbol");
 }
 
 } // namespace facebook::velox::functions::test

velox/common/memory/MemoryAllocator.h

@@ -70,7 +70,7 @@ struct SizeClassStats {
     SizeClassStats result;
     result.size = size;
     result.allocateClocks = allocateClocks - other.allocateClocks;
-    result.allocateClocks = freeClocks - other.freeClocks;
+    result.freeClocks = freeClocks - other.freeClocks;
     result.numAllocations = numAllocations - other.numAllocations;
     result.totalBytes = totalBytes - other.totalBytes;
     return result;

velox/common/memory/MemoryArbitrator.h

@@ -16,6 +16,7 @@
 
 #pragma once
 
+#include <algorithm>
 #include <vector>
 
 #include "velox/common/base/AsyncSource.h"
@@ -93,9 +94,26 @@ class MemoryArbitrator {
     std::unordered_map<std::string, std::string> extraConfigs{};
 
     std::string toString() const {
-      std::stringstream ss;
+      // Emit the extra configs in key order. Iterating the unordered map
+      // directly makes the output depend on the hash implementation, which
+      // differs between standard libraries and would make error messages and
+      // logs vary by platform.
+      std::vector<const std::pair<const std::string, std::string>*>
+          sortedConfigs;
+      sortedConfigs.reserve(extraConfigs.size());
       for (const auto& extraConfig : extraConfigs) {
-        ss << extraConfig.first << "=" << extraConfig.second << ";";
+        sortedConfigs.push_back(&extraConfig);
+      }
+      std::sort(
+          sortedConfigs.begin(),
+          sortedConfigs.end(),
+          [](const auto* lhs, const auto* rhs) {
+            return lhs->first < rhs->first;
+          });
+
+      std::stringstream ss;
+      for (const auto* extraConfig : sortedConfigs) {
+        ss << extraConfig->first << "=" << extraConfig->second << ";";
       }
       return fmt::format(
           "kind={};capacity={};arbitrationStateCheckCb={};{}",

velox/common/memory/tests/MemoryAllocatorTest.cpp

@@ -664,6 +664,30 @@ TEST_P(MemoryAllocatorTest, allocationClass2) {
   allocation->clear();
 }
 
+TEST_P(MemoryAllocatorTest, sizeClassStatsDifference) {
+  SizeClassStats newer;
+  newer.size = 8;
+  newer.allocateClocks = 500;
+  newer.freeClocks = 70;
+  newer.numAllocations = 9;
+  newer.totalBytes = 4'096;
+
+  SizeClassStats older;
+  older.size = 8;
+  older.allocateClocks = 200;
+  older.freeClocks = 20;
+  older.numAllocations = 4;
+  older.totalBytes = 1'024;
+
+  const auto delta = newer - older;
+  EXPECT_EQ(delta.size, 8);
+  EXPECT_EQ(delta.allocateClocks, 300);
+  EXPECT_EQ(delta.freeClocks, 50);
+  EXPECT_EQ(delta.numAllocations, 5);
+  EXPECT_EQ(delta.totalBytes, 3'072);
+  EXPECT_EQ(delta.clocks(), 350);
+}
+
 TEST_P(MemoryAllocatorTest, stats) {
   const std::vector<MachinePageCount>& sizes = instance_->sizeClasses();
   for (auto i = 0; i < sizes.size(); ++i) {
@@ -681,11 +705,17 @@ TEST_P(MemoryAllocatorTest, stats) {
   gflags::FlagSaver flagSaver;
   FLAGS_velox_time_allocations = true;
   for (auto i = 0; i < sizes.size(); ++i) {
-    std::unique_ptr<Allocation> allocation = std::make_unique<Allocation>();
     auto size = sizes[i];
-    ASSERT_TRUE(allocate(size, *allocation));
-    ASSERT_GT(instance_->numAllocated(), 0);
-    instance_->freeNonContiguous(*allocation);
+    // A single allocate/free pair can take less than one tick of the hardware
+    // timestamp counter, whose resolution varies by platform (for example it
+    // is far coarser on ARM than the x86 TSC). Repeat so the accumulated time
+    // is measurable everywhere.
+    for (auto repeat = 0; repeat < 64; ++repeat) {
+      std::unique_ptr<Allocation> allocation = std::make_unique<Allocation>();
+      ASSERT_TRUE(allocate(size, *allocation));
+      ASSERT_GT(instance_->numAllocated(), 0);
+      instance_->freeNonContiguous(*allocation);
+    }
     auto stats = instance_->stats();
     ASSERT_LT(0, stats.sizes[i].clocks());
     ASSERT_GE(stats.sizes[i].totalBytes, size * AllocationTraits::kPageSize);

velox/common/memory/tests/MemoryCapExceededTest.cpp

@@ -75,11 +75,14 @@ TEST_P(MemoryCapExceededTest, singleDriver) {
       "numSucceded 0 numAborted 0 numFailures 0 numNonReclaimableAttempts 0 "
       "reclaimedFreeCapacity 0B reclaimedUsedCapacity 0B maxCapacity 6.00GB "
       "freeCapacity 5.50GB freeReservedCapacity 0B] CONFIG[kind=SHARED;"
+      // Extra configs are emitted in key order.
       "capacity=6.00GB;arbitrationStateCheckCb=(set);"
-      "memory-pool-abort-capacity-limit=0B;memory-pool-min-reclaim-pct=0;"
-      "memory-pool-reserved-capacity=0B;"
+      "global-arbitration-enabled=true;"
+      "memory-pool-abort-capacity-limit=0B;"
       "memory-pool-initial-capacity=536870912B;"
-      "global-arbitration-enabled=true;memory-pool-min-reclaim-bytes=0B;"
+      "memory-pool-min-reclaim-bytes=0B;"
+      "memory-pool-min-reclaim-pct=0;"
+      "memory-pool-reserved-capacity=0B;"
       "reserved-capacity=0B;]]"
       "\n\n"
       "Memory Pool[",

velox/common/memory/tests/MockSharedArbitratorTest.cpp

@@ -630,20 +630,21 @@ TEST_F(MockSharedArbitrationTest, configToString) {
       "SHARED", 1024, nullptr, std::move(configs)};
   ASSERT_EQ(
       arbitratorConfig.toString(),
+      // Extra configs are emitted in key order.
       "kind=SHARED;capacity=1.00KB;"
       "arbitrationStateCheckCb=(unset);"
-      "global-arbitration-without-spill=true;"
-      "memory-reclaim-threads-hw-multiplier=1.0;"
-      "memory-pool-min-reclaim-pct=0.3;"
       "check-usage-leak=false;"
+      "global-arbitration-abort-time-ratio=0.8;"
       "global-arbitration-enabled=true;"
-      "max-memory-arbitration-time=5000ms;"
       "global-arbitration-memory-reclaim-pct=30;"
+      "global-arbitration-without-spill=true;"
+      "max-memory-arbitration-time=5000ms;"
       "memory-pool-abort-capacity-limit=256mb;"
+      "memory-pool-initial-capacity=512MB;"
       "memory-pool-min-reclaim-bytes=64mb;"
+      "memory-pool-min-reclaim-pct=0.3;"
       "memory-pool-reserved-capacity=200B;"
-      "memory-pool-initial-capacity=512MB;"
-      "global-arbitration-abort-time-ratio=0.8;"
+      "memory-reclaim-threads-hw-multiplier=1.0;"
       "reserved-capacity=100B;");
 }
 

velox/dwio/dwrf/test/CacheInputTest.cpp

@@ -72,12 +72,16 @@ class CacheTest : public ::testing::Test {
   }
 
   void shutdownCache() {
-    if (cache_ != nullptr) {
-      cache_->shutdown();
-    }
+    // Join the IO executor before shutting the cache down. Prefetches run on
+    // the executor and pin cache entries, so a task that is still in flight
+    // would otherwise touch a cache that has already been shut down.
     if (executor_ != nullptr) {
+      executor_->join();
       executor_.reset();
     }
+    if (cache_ != nullptr) {
+      cache_->shutdown();
+    }
     if (cache_ != nullptr) {
       auto* ssdCache = cache_->ssdCache();
       if (ssdCache != nullptr) {

velox/exec/benchmarks/RowContainerSortBenchmark.cpp

@@ -61,7 +61,10 @@
       opts);
 }
 
-std::vector<std::optional<StringView>> getDataFromFile() {
+// Returns the merged strings read from the sample file. The strings are
+// returned by value because a StringView does not own its characters, so the
+// caller has to keep them alive for as long as it uses the views built below.
+std::vector<std::string> getDataFromFile() {
   const std::string sample(getExampleFilePath("str_sort.parquet"));
   auto rowType = ROW({"query_sig", "result_sig"}, {VARCHAR(), VARCHAR()});
   auto pool = memory::memoryManager()->addLeafPool();
@@ -78,17 +81,19 @@
   auto rowReader = reader.createRowReader(rowReaderOpts);
   auto data = BaseVector::create(rowType, 50000, pool.get());
   rowReader->next(50000, data);
-  auto querySigCol =
-      data->as<RowVector>()->childAt(0)->asFlatVector<StringView>();
-  auto resSigCol =
-      data->as<RowVector>()->childAt(1)->asFlatVector<StringView>();
-  std::vector<std::optional<StringView>> stdVector(querySigCol->size());
-  for (int i = 0; i < querySigCol->size(); i++) {
-    auto merge =
-        querySigCol->valueAt(i).getString() + resSigCol->valueAt(i).getString();
-    stdVector[i] = StringView(merge);
+  // The columns are not necessarily flat, so decode them instead of assuming
+  // an encoding. asFlatVector() returns null for a dictionary encoded column.
+  auto* rowVector = data->as<RowVector>();
+  SelectivityVector rows(rowVector->size());
+  DecodedVector querySigCol(*rowVector->childAt(0), rows);
+  DecodedVector resSigCol(*rowVector->childAt(1), rows);
+
+  std::vector<std::string> merged(rowVector->size());
+  for (auto i = 0; i < rowVector->size(); ++i) {
+    merged[i] = querySigCol.valueAt<StringView>(i).getString() +
+        resSigCol.valueAt<StringView>(i).getString();
   }
-  return stdVector;
+  return merged;
 }
 
 std::vector<char*> store(
@@ -173,7 +178,12 @@
   folly::BenchmarkSuspender suspender;
   auto pool = memory::memoryManager()->addLeafPool();
   VectorMaker vectorMaker(pool.get());
-  auto data = getDataFromFile();
+  const auto strings = getDataFromFile();
+  std::vector<std::optional<StringView>> data;
+  data.reserve(strings.size());
+  for (const auto& value : strings) {
+    data.push_back(StringView(value));
+  }
   auto vector =
       vectorMaker.encodedVector<StringView>(VectorEncoding::Simple::FLAT, data);
   DecodedVector decoded(*vector);
@@ -198,7 +208,12 @@
   folly::BenchmarkSuspender suspender;
   auto pool = memory::memoryManager()->addLeafPool();
   VectorMaker vectorMaker(pool.get());
-  auto data = getDataFromFile();
+  const auto strings = getDataFromFile();
+  std::vector<std::optional<StringView>> data;
+  data.reserve(strings.size());
+  for (const auto& value : strings) {
+    data.push_back(StringView(value));
+  }
   auto vector =
       vectorMaker.encodedVector<StringView>(VectorEncoding::Simple::FLAT, data);
   DecodedVector decoded(*vector);

velox/exec/tests/MultiFragmentTest.cpp

@@ -564,7 +564,13 @@ TEST_P(MultiFragmentTest, abortMergeExchange) {
 
   for (auto& task : tasks) {
     task->requestAbort();
-    ASSERT_TRUE(waitForTaskAborted(task.get())) << task->taskId();
+    // A partial sort task can run to completion before it observes the abort
+    // request, in which case it terminates as finished rather than aborted.
+    // Both are terminal states, and what this test needs is only that the task
+    // stopped running and its drivers finished.
+    ASSERT_TRUE(
+        waitForTaskAborted(task.get()) || waitForTaskCompletion(task.get()))
+        << task->toString();
   }
 
   // Ensure that the threads in the executor can gracefully join

velox/exec/tests/TableScanTest.cpp

@@ -6893,7 +6893,13 @@ TEST_F(TableScanTest, parallelUnitLoader) {
 }
 
 TEST_F(TableScanTest, filterColumnHandles) {
-  auto data = makeVectors(1, 10, ROW({"a", "b"}, BIGINT()));
+  // The remaining filter below evaluates to null, and therefore drops the row,
+  // whenever 'a' is null. Use data without nulls so that every row is expected
+  // in the result regardless of what random data would have been generated.
+  std::vector<RowVectorPtr> data{makeRowVector(
+      {"a", "b"},
+      {makeFlatVector<int64_t>(10, folly::identity),
+       makeFlatVector<int64_t>(10, folly::identity)})};
   auto filePath = TempFilePath::create();
   writeToFile(filePath->getPath(), data);
   auto split = exec::test::HiveConnectorSplitBuilder(filePath->getPath())