Merge branch 'main' into checked-decimal-multiply

Resolve conflicts with merged checked_add/checked_subtract (PR facebookincubator#16302).
Update multiply tests to use generic checkedDecimalArithmetic helpers.

Author: n0r0shi

.github/workflows/claude-review.yml

@@ -35,7 +35,7 @@ concurrency:
 jobs:
   build_docs:
     name: Build and Push
-    runs-on: 8-core-ubuntu
+    runs-on: 16-core-ubuntu
     container: ghcr.io/facebookincubator/velox-dev:centos9
     env:
       CCACHE_DIR: /tmp/ccache
@@ -48,25 +48,71 @@ jobs:
         id: restore-cache
         with:
           path: ${{ env.CCACHE_DIR }}
-          key: ccache-docs-8-core-ubuntu
+          key: ccache-docs-16-core-ubuntu
 
       - name: Checkout
         uses: actions/checkout@08c6903cd8c0fde910a37f88322edcfb5dd907a8 # v5.0.0
         with:
           fetch-depth: 0
           persist-credentials: true
 
+      - name: Configure git safe directory
+        run: git config --global --add safe.directory ${GITHUB_WORKSPACE}
+
       - name: Install System Dependencies
         run: |
           dnf install -y --setopt=install_weak_deps=False pandoc
 
       - name: CCache Stats Before
         run: ccache -sz
 
+      - name: Install uv
+        run: |
+          pip install --upgrade uv
+          uv --version
+
+      - name: Check for pyvelox changes
+        id: check-pyvelox
+        run: |
+          if [ "${{ github.event_name }}" = "pull_request" ]; then
+            CHANGED=$(git diff --name-only ${{ github.event.pull_request.base.sha }}..HEAD)
+          else
+            CHANGED=$(git diff --name-only HEAD~1..HEAD)
+          fi
+          if echo "$CHANGED" | grep -qE '^(python/|velox/python/)'; then
+            echo "Building pyvelox: python/ files changed."
+            echo "changed=true" >> $GITHUB_OUTPUT
+          else
+            echo "Skipping pyvelox build: no python/ files changed."
+            echo "changed=false" >> $GITHUB_OUTPUT
+          fi
+
       - name: Install Python Dependencies
+        timeout-minutes: 30
+        env:
+          PYVELOX_CHANGED: ${{ steps.check-pyvelox.outputs.changed }}
         run: |
-          # Install pyvelox to generate it's docs
-          uv sync --extra docs
+          # When python/ files haven't changed, skip building pyvelox
+          # from C++ source (slow) and only install doc dependencies.
+          EXTRA_FLAGS=""
+          if [ "$PYVELOX_CHANGED" != "true" ]; then
+            echo "Skipping pyvelox C++ build (no changes in python/)."
+            EXTRA_FLAGS="--no-install-project"
+          fi
+          # Retry up to 3 times to handle transient network issues
+          # (pip/uv hangs, registry timeouts).
+          for attempt in 1 2 3; do
+            echo "Attempt $attempt of 3..."
+            if uv sync --extra docs $EXTRA_FLAGS --verbose; then
+              echo "Python dependencies installed successfully."
+              exit 0
+            fi
+            echo "Attempt $attempt failed. Cleaning up and retrying in 10 seconds..."
+            rm -rf .venv uv.lock
+            sleep 10
+          done
+          echo "All 3 attempts failed."
+          exit 1
 
       - name: CCache Stats After
         run: ccache -s
@@ -75,7 +121,7 @@ jobs:
         uses: apache/infrastructure-actions/stash/save@3354c1565d4b0e335b78a76aedd82153a9e144d4
         with:
           path: ${{ env.CCACHE_DIR }}
-          key: ccache-docs-8-core-ubuntu
+          key: ccache-docs-16-core-ubuntu
 
       - name: Build Documentation
         run: |
@@ -92,7 +138,6 @@ jobs:
         run: |
           git config --global user.email "velox@users.noreply.github.com"
           git config --global user.name "velox"
-          git config --global --add safe.directory ${GITHUB_WORKSPACE}
 
       - name: Push Documentation
         if: ${{ github.event_name == 'push' && github.repository == 'facebookincubator/velox'}}

.github/workflows/docs.yml

@@ -670,6 +670,9 @@ if("${CMAKE_CXX_COMPILER_ID}" MATCHES "GNU")
   # Find Threads library
   find_package(Threads REQUIRED)
   add_compile_options($<$<COMPILE_LANGUAGE:CXX>:-fcoroutines>)
+  # Explicitly enable folly coroutines when compiler supports them.
+  # This is required for folly::coro::AsyncGenerator to work correctly.
+  add_compile_definitions(FOLLY_HAS_COROUTINES=1)
 endif()
 
 if(VELOX_BUILD_TESTING AND NOT VELOX_ENABLE_DUCKDB)

CMakeLists.txt

@@ -107,6 +107,13 @@ target_link_libraries(
 add_executable(velox_cast_benchmark CastBenchmark.cpp)
 target_link_libraries(velox_cast_benchmark ${velox_benchmark_deps} velox_vector_test_lib)
 
+add_executable(velox_benchmark_expr_flat_no_nulls ExprFlatNoNullsBenchmark.cpp)
+target_link_libraries(
+  velox_benchmark_expr_flat_no_nulls
+  ${velox_benchmark_deps}
+  velox_functions_prestosql
+)
+
 add_executable(velox_format_datetime_benchmark FormatDateTimeBenchmark.cpp)
 target_link_libraries(
   velox_format_datetime_benchmark

velox/benchmarks/basic/CMakeLists.txt

@@ -0,0 +1,121 @@
+/*
+ * Copyright (c) Facebook, Inc. and its affiliates.
+ *
+ * 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.
+ */
+
+/// Benchmark comparing expression evaluation with FlatNoNulls fast path
+/// enabled vs disabled via the expression.eval_flat_no_nulls config.
+
+#include <folly/Benchmark.h>
+#include <folly/init/Init.h>
+
+#include "velox/benchmarks/ExpressionBenchmarkBuilder.h"
+#include "velox/core/QueryConfig.h"
+#include "velox/functions/prestosql/registration/RegistrationFunctions.h"
+
+using namespace facebook;
+using namespace facebook::velox;
+
+namespace {
+
+void addArithmeticBenchmarks(
+    ExpressionBenchmarkBuilder& builder,
+    const std::string& prefix) {
+  for (auto vectorSize : {1'024, 4'096, 40'960, 99'999}) {
+    builder
+        .addBenchmarkSet(
+            fmt::format("{}_arith_batch{}", prefix, vectorSize),
+            ROW({"a", "b", "c", "d"}, {DOUBLE(), DOUBLE(), DOUBLE(), DOUBLE()}))
+        .withFuzzerOptions(
+            {.vectorSize = static_cast<size_t>(vectorSize), .nullRatio = 0.0})
+        // Simple arithmetic — 1 node.
+        .addExpression("add_ab", "a + b")
+        // Complex arithmetic — 7 nodes.
+        .addExpression("complex_7n", "(a + b) * c + (a - d) * b")
+        // Deep tree — 15 nodes, depth 8 (left-skewed chain).
+        .addExpression(
+            "deep_15n_d8", "((((((a + b) * c - d) + a) * b - c) + d) * a - b)")
+        .withIterations(1'000);
+  }
+}
+
+void addComparisonBenchmarks(
+    ExpressionBenchmarkBuilder& builder,
+    const std::string& prefix) {
+  for (auto vectorSize : {1'024, 4'096, 40'960, 99'999}) {
+    builder
+        .addBenchmarkSet(
+            fmt::format("{}_cmp_batch{}", prefix, vectorSize),
+            ROW({"a", "b"}, {DOUBLE(), DOUBLE()}))
+        .withFuzzerOptions(
+            {.vectorSize = static_cast<size_t>(vectorSize), .nullRatio = 0.0})
+        // Comparison — 1 node.
+        .addExpression("eq_ab", "a = b")
+        .withIterations(1'000);
+  }
+}
+
+void addConstMixedBenchmarks(
+    ExpressionBenchmarkBuilder& builder,
+    const std::string& prefix) {
+  for (auto vectorSize : {1'024, 4'096, 40'960, 99'999}) {
+    builder
+        .addBenchmarkSet(
+            fmt::format("{}_const_batch{}", prefix, vectorSize),
+            ROW({"a", "b"}, {DOUBLE(), DOUBLE()}))
+        .withFuzzerOptions(
+            {.vectorSize = static_cast<size_t>(vectorSize), .nullRatio = 0.0})
+        // 1 constant.
+        .addExpression("const_1", "a + 1.5")
+        // 2 constants.
+        .addExpression("const_2", "(a + 1.5) * 2.0")
+        // 3 constants mixed with columns — 7 nodes.
+        .addExpression("const_3_7n", "(a + 1.5) * 2.0 + (a - 3.0) * b")
+        .withIterations(1'000);
+  }
+}
+
+/// Extends ExpressionBenchmarkBuilder to allow setting query config.
+class ConfigurableBenchmarkBuilder : public ExpressionBenchmarkBuilder {
+ public:
+  void setConfig(const std::string& key, const std::string& value) {
+    queryCtx_->testingOverrideConfigUnsafe({{key, value}});
+  }
+};
+
+} // namespace
+
+int main(int argc, char** argv) {
+  folly::Init init(&argc, &argv);
+  memory::MemoryManager::initialize(memory::MemoryManager::Options{});
+  functions::prestosql::registerAllScalarFunctions();
+
+  // Fast path ON (default).
+  ConfigurableBenchmarkBuilder fastPathOn;
+  addArithmeticBenchmarks(fastPathOn, "on");
+  addComparisonBenchmarks(fastPathOn, "on");
+  addConstMixedBenchmarks(fastPathOn, "on");
+
+  // Fast path OFF via config.
+  ConfigurableBenchmarkBuilder fastPathOff;
+  fastPathOff.setConfig(core::QueryConfig::kExprEvalFlatNoNulls, "false");
+  addArithmeticBenchmarks(fastPathOff, "off");
+  addComparisonBenchmarks(fastPathOff, "off");
+  addConstMixedBenchmarks(fastPathOff, "off");
+
+  fastPathOn.registerBenchmarks();
+  fastPathOff.registerBenchmarks();
+  folly::runBenchmarks();
+  return 0;
+}

velox/benchmarks/basic/ExprFlatNoNullsBenchmark.cpp

@@ -110,6 +110,34 @@ class BloomFilter {
     }
   }
 
+  /// Computes m (total bits of Bloom filter) which is expected to achieve,
+  /// for the specified expected insertions, the required false positive
+  /// probability.
+  ///
+  /// See
+  /// http://en.wikipedia.org/wiki/Bloom_filter#Probability_of_false_positives
+  /// for the formula.
+  ///
+  /// @param n expected insertions (must be positive).
+  /// @param p false positive rate (must be 0 < p < 1).
+  static int64_t optimalNumOfBits(int64_t n, double p) {
+    return static_cast<int64_t>(
+        -n * std::log(p) / (std::log(2.0) * std::log(2.0)));
+  }
+
+  /// Computes m (total bits of Bloom filter) which is expected to achieve.
+  /// The smaller the expectedNumItems, the smaller the fpp.
+  ///
+  /// @param expectedNumItems expected number of items to insert.
+  /// @param maxNumItems maximum number of items.
+  static int64_t optimalNumOfBits(
+      int64_t expectedNumItems,
+      int64_t maxNumItems) {
+    double ratio = static_cast<double>(expectedNumItems) / maxNumItems;
+    double fpp = kDefaultFpp * std::min(ratio, 1.0);
+    return optimalNumOfBits(expectedNumItems, fpp);
+  }
+
  private:
   // We use 4 independent hash functions by taking 24 bits of
   // the hash code and breaking these up into 4 groups of 6 bits. Each group
@@ -142,6 +170,8 @@ class BloomFilter {
   }
 
   static constexpr int8_t kBloomFilterV1 = 1;
+  static constexpr double kDefaultFpp = 0.03;
+
   std::vector<uint64_t, Allocator> bits_;
 };
 

velox/common/base/BloomFilter.h

@@ -113,7 +113,7 @@ BaseRuntimeStatWriter* getThreadLocalRunTimeStatWriter() {
 }
 
 void addThreadLocalRuntimeStat(
-    const std::string& name,
+    std::string_view name,
     const RuntimeCounter& value) {
   if (localRuntimeStatWriter) {
     localRuntimeStatWriter->addRuntimeStat(name, value);

velox/common/base/RuntimeMetrics.cpp

@@ -19,6 +19,7 @@
 #include <fmt/format.h>
 #include <folly/CppAttributes.h>
 #include <limits>
+#include <string_view>
 
 namespace facebook::velox {
 
@@ -77,7 +78,7 @@ class BaseRuntimeStatWriter {
   virtual ~BaseRuntimeStatWriter() = default;
 
   virtual void addRuntimeStat(
-      const std::string& /* name */,
+      std::string_view /* name */,
       const RuntimeCounter& /* value */) {}
 };
 
@@ -93,7 +94,7 @@ BaseRuntimeStatWriter* getThreadLocalRunTimeStatWriter();
 
 /// Writes runtime counter to the current Operator running on that thread.
 void addThreadLocalRuntimeStat(
-    const std::string& name,
+    std::string_view name,
     const RuntimeCounter& value);
 
 /// Scope guard to conveniently set and revert back the current stat writer.

velox/common/base/RuntimeMetrics.h

@@ -122,3 +122,28 @@ TEST_F(BloomFilterTest, merge) {
 
   EXPECT_EQ(bloom.serializedSize(), merge.serializedSize());
 }
+
+TEST_F(BloomFilterTest, optimalNumOfBitsWithFpp) {
+  EXPECT_EQ(BloomFilter<>::optimalNumOfBits(1000, 0.03), 7298);
+  EXPECT_EQ(BloomFilter<>::optimalNumOfBits(1000000, 0.01), 9585058);
+  EXPECT_EQ(BloomFilter<>::optimalNumOfBits(1, 0.5), 1);
+  EXPECT_EQ(BloomFilter<>::optimalNumOfBits(1000, 0.001), 14377);
+}
+
+TEST_F(BloomFilterTest, optimalNumOfBitsWithMaxItems) {
+  constexpr int64_t kMaxNumItems = 4'000'000L;
+
+  EXPECT_EQ(
+      BloomFilter<>::optimalNumOfBits(kMaxNumItems, kMaxNumItems), 29'193'763);
+
+  EXPECT_EQ(
+      BloomFilter<>::optimalNumOfBits(1'000'000L, kMaxNumItems), 10'183'830);
+
+  EXPECT_EQ(BloomFilter<>::optimalNumOfBits(100L, kMaxNumItems), 2935);
+
+  EXPECT_EQ(
+      BloomFilter<>::optimalNumOfBits(5'000'000L, kMaxNumItems), 36'492'204);
+
+  EXPECT_EQ(
+      BloomFilter<>::optimalNumOfBits(10'000'000L, kMaxNumItems), 72'984'408);
+}