Remove config

PHILO-HE · Jun 7, 2024 · 6cb9eee · 6cb9eee
1 parent c87993f
commit 6cb9eee
Show file tree

Hide file tree

Showing 5 changed files with 32 additions and 71 deletions.
diff --git a/velox/core/QueryConfig.h b/velox/core/QueryConfig.h
@@ -284,10 +284,6 @@ class QueryConfig {
   static constexpr const char* kPrestoArrayAggIgnoreNulls =
       "presto.array_agg.ignore_nulls";
 
-  /// If false, size function returns null for null input.
-  static constexpr const char* kSparkLegacySizeOfNull =
-      "spark.legacy_size_of_null";
-
   // The default number of expected items for the bloomfilter.
   static constexpr const char* kSparkBloomFilterExpectedNumItems =
       "spark.bloom_filter.expected_num_items";
@@ -613,11 +609,6 @@ class QueryConfig {
     return get<int32_t>(kSpillableReservationGrowthPct, kDefaultPct);
   }
 
-  bool sparkLegacySizeOfNull() const {
-    constexpr bool kDefault{true};
-    return get<bool>(kSparkLegacySizeOfNull, kDefault);
-  }
-
   bool prestoArrayAggIgnoreNulls() const {
     return get<bool>(kPrestoArrayAggIgnoreNulls, false);
   }

diff --git a/velox/docs/functions/spark/array.rst b/velox/docs/functions/spark/array.rst
@@ -161,17 +161,10 @@ Array Functions
         SELECT shuffle(array(0, 0, 0), 0); -- [0, 0, 0]
         SELECT shuffle(array(1, NULL, 1, NULL, 2), 0); -- [2, 1, NULL, NULL, 1]
 
-.. spark:function:: size(array(E)) -> bigint
+.. spark:function:: size(array(E), legacySizeOfNull) -> int
 
-    Returns the size of the array. Returns null for null input
-    if :doc:`spark.legacy_size_of_null <../../configs>` is set to false.
-    Otherwise, returns -1 for null input.
-
-.. spark:function:: size(array(E), legacyOfNull) -> bigint
-
-    Returns the size of the array. Returns null for null input if `legacyOfNull`
-    is set to false, which is the behavior of Spark's `array_size` function.
-    Otherwise, returns -1 for null input.
+    Returns the size of the array. Returns null for null input if `legacySizeOfNull`
+    is set to false. Otherwise, returns -1 for null input.
 
 .. spark:function:: sort_array(array(E)) -> array(E)
 

diff --git a/velox/docs/functions/spark/map.rst b/velox/docs/functions/spark/map.rst
@@ -41,9 +41,8 @@ Map Functions
 
     Returns all the values in the map ``x``.
 
-.. spark:function:: size(map(K,V)) -> bigint
+.. spark:function:: size(map(K,V), legacySizeOfNull) -> int
    :noindex:
 
-    Returns the size of the input map. Returns null for null input
-    if :doc:`spark.legacy_size_of_null <../../configs>` is set to false.
-    Otherwise, returns -1 for null input.
+    Returns the size of the input map. Returns null for null input if ``legacySizeOfNull``
+    is set to false. Otherwise, returns -1 for null input.
diff --git a/velox/functions/sparksql/Size.cpp b/velox/functions/sparksql/Size.cpp
@@ -30,20 +30,11 @@ struct Size {
   template <typename TInput>
   FOLLY_ALWAYS_INLINE void initialize(
       const std::vector<TypePtr>& /*inputTypes*/,
-      const core::QueryConfig& config,
-      const TInput* input /*input*/) {
-    legacySizeOfNull_ = config.sparkLegacySizeOfNull();
-  }
-
-  template <typename TInput>
-  FOLLY_ALWAYS_INLINE void initialize(
-      const std::vector<TypePtr>& /*inputTypes*/,
-      const core::QueryConfig& config,
+      const core::QueryConfig& /*config*/,
       const TInput* /*input*/,
       const bool* legacySizeOfNull) {
     VELOX_USER_CHECK_NE(
         legacySizeOfNull, nullptr, "Constant legacySizeOfNull is expected.");
-    // Respects the passed legacySizeOfNull.
     legacySizeOfNull_ = *legacySizeOfNull;
   }
 
@@ -66,7 +57,16 @@ struct Size {
       int32_t& out,
       const TInput* input,
       const bool* /*legacySizeOfNull*/) {
-    return callNullable(out, input);
+    if (input == nullptr) {
+      if (legacySizeOfNull_) {
+        out = -1;
+        return true;
+      } else {
+        return false;
+      }
+    }
+    out = input->size();
+    return true;
   }
 
  private:
@@ -75,10 +75,8 @@ struct Size {
 } // namespace
 
 void registerSize(const std::string& prefix) {
-  registerFunction<Size, int32_t, Array<Any>>({prefix});
-  registerFunction<Size, int32_t, Map<Any, Any>>({prefix});
-  // Register with legacySizeOfNull.
   registerFunction<Size, int32_t, Array<Any>, bool>({prefix});
+  registerFunction<Size, int32_t, Map<Any, Any>, bool>({prefix});
 }
 
 } // namespace facebook::velox::functions::sparksql
diff --git a/velox/functions/sparksql/tests/SizeTest.cpp b/velox/functions/sparksql/tests/SizeTest.cpp
@@ -28,9 +28,9 @@ class SizeTest : public SparkFunctionBaseTest {
   std::function<vector_size_t(vector_size_t /* row */)> sizeAt =
       [](vector_size_t row) { return 1 + row % 7; };
 
-  void testSize(VectorPtr vector, vector_size_t numRows) {
-    auto result =
-        evaluate<SimpleVector<int32_t>>("size(c0)", makeRowVector({vector}));
+  void testLegacySizeOfNull(VectorPtr vector, vector_size_t numRows) {
+    auto result = evaluate<SimpleVector<int32_t>>(
+        "size(c0, true)", makeRowVector({vector}));
     for (vector_size_t i = 0; i < numRows; ++i) {
       if (vector->isNullAt(i)) {
         EXPECT_EQ(result->valueAt(i), -1) << "at " << i;
@@ -40,15 +40,7 @@ class SizeTest : public SparkFunctionBaseTest {
     }
   }
 
-  void testSizeConfiguredLegacyNull(VectorPtr vector, vector_size_t numRows) {
-    auto result =
-        evaluate<SimpleVector<int32_t>>("size(c0)", makeRowVector({vector}));
-    for (vector_size_t i = 0; i < numRows; ++i) {
-      EXPECT_EQ(result->isNullAt(i), vector->isNullAt(i)) << "at " << i;
-    }
-  }
-
-  void testSizePassedLegacyNull(VectorPtr vector, vector_size_t numRows) {
+  void testSize(VectorPtr vector, vector_size_t numRows) {
     auto result = evaluate<SimpleVector<int32_t>>(
         "size(c0, false)", makeRowVector({vector}));
     for (vector_size_t i = 0; i < numRows; ++i) {
@@ -67,44 +59,32 @@ class SizeTest : public SparkFunctionBaseTest {
   }
 };
 
-TEST_F(SizeTest, sizetest) {
+// Ensure that out is set to -1 for null input if legacySizeOfNull = true.
+TEST_F(SizeTest, legacySizeOfNull) {
   vector_size_t numRows = 100;
   auto arrayVector =
       makeArrayVector<int64_t>(numRows, sizeAt, valueAt, nullptr);
-  testSize(arrayVector, numRows);
+  testLegacySizeOfNull(arrayVector, numRows);
   arrayVector =
       makeArrayVector<int64_t>(numRows, sizeAt, valueAt, nullEvery(5));
-  testSize(arrayVector, numRows);
+  testLegacySizeOfNull(arrayVector, numRows);
   auto mapVector = makeMapVector<int64_t, int64_t>(
       numRows, sizeAt, valueAt, valueAt, nullptr);
-  testSize(mapVector, numRows);
+  testLegacySizeOfNull(mapVector, numRows);
   mapVector = makeMapVector<int64_t, int64_t>(
       numRows, sizeAt, valueAt, valueAt, nullEvery(5));
-  testSize(mapVector, numRows);
+  testLegacySizeOfNull(mapVector, numRows);
 }
 
-// Ensure that out is set to null for null input if configured
-// SparkLegacySizeOfNull is false.
-TEST_F(SizeTest, configuredLegacySizeOfNull) {
+// Ensure that out is set to null for null input if legacySizeOfNull = false.
+TEST_F(SizeTest, notLegacySizeOfNull) {
   vector_size_t numRows = 100;
-  setConfig(core::QueryConfig::kSparkLegacySizeOfNull, false);
   auto arrayVector =
       makeArrayVector<int64_t>(numRows, sizeAt, valueAt, nullEvery(1));
-  testSizeConfiguredLegacyNull(arrayVector, numRows);
+  testSize(arrayVector, numRows);
   auto mapVector = makeMapVector<int64_t, int64_t>(
       numRows, sizeAt, valueAt, valueAt, nullEvery(1));
-  testSizeConfiguredLegacyNull(mapVector, numRows);
-}
-
-// Ensure that out is set to null for null input if passed legacySizeOfNull is
-// false, regardless of the configuration.
-TEST_F(SizeTest, passedLegacySizeOfNull) {
-  vector_size_t numRows = 100;
-  // Dismiss the configuration.
-  setConfig(core::QueryConfig::kSparkLegacySizeOfNull, true);
-  auto arrayVector =
-      makeArrayVector<int64_t>(numRows, sizeAt, valueAt, nullEvery(1));
-  testSizePassedLegacyNull(arrayVector, numRows);
+  testSize(mapVector, numRows);
 }
 
 } // namespace facebook::velox::functions::sparksql::test