Back out "Back out "[velox][PR] Add support for decimals in approx_di…

…stinct aggregate function"" (facebookincubator#7904) Summary: Pull Request resolved: facebookincubator#7904 Differential Revision: D51913350
kagamiori · Dec 6, 2023 · 80d266c · 80d266c
1 parent e963545
commit 80d266c
Show file tree

Hide file tree

Showing 2 changed files with 55 additions and 6 deletions.
diff --git a/velox/functions/prestosql/aggregates/ApproxDistinctAggregate.cpp b/velox/functions/prestosql/aggregates/ApproxDistinctAggregate.cpp
@@ -437,6 +437,7 @@ exec::AggregateRegistrationResult registerApproxDistinct(
           "smallint",
           "integer",
           "bigint",
+          "hugeint",
           "real",
           "double",
           "varchar",
@@ -455,6 +456,21 @@ exec::AggregateRegistrationResult registerApproxDistinct(
                                .argumentType("double")
                                .build());
     }
+    signatures.push_back(exec::AggregateFunctionSignatureBuilder()
+                             .typeVariable("a_precision")
+                             .typeVariable("a_scale")
+                             .returnType(returnType)
+                             .intermediateType("varbinary")
+                             .argumentType("DECIMAL(a_precision, a_scale)")
+                             .build());
+    signatures.push_back(exec::AggregateFunctionSignatureBuilder()
+                             .typeVariable("a_precision")
+                             .typeVariable("a_scale")
+                             .returnType(returnType)
+                             .intermediateType("varbinary")
+                             .argumentType("DECIMAL(a_precision, a_scale)")
+                             .argumentType("double")
+                             .build());
   }
 
   return exec::registerAggregateFunction(

diff --git a/velox/functions/prestosql/aggregates/tests/ApproxDistinctTest.cpp b/velox/functions/prestosql/aggregates/tests/ApproxDistinctTest.cpp
@@ -32,7 +32,8 @@ class ApproxDistinctTest : public AggregationTestBase {
   void testGlobalAgg(
       const VectorPtr& values,
       double maxStandardError,
-      int64_t expectedResult) {
+      int64_t expectedResult,
+      bool testWithTableScan = true) {
     auto vectors = makeRowVector({values});
     auto expected =
         makeRowVector({makeNullableFlatVector<int64_t>({expectedResult})});
@@ -41,7 +42,9 @@ class ApproxDistinctTest : public AggregationTestBase {
         {vectors},
         {},
         {fmt::format("approx_distinct(c0, {})", maxStandardError)},
-        {expected});
+        {expected},
+        {},
+        testWithTableScan);
     testAggregationsWithCompanion(
         {vectors},
         [](auto& /*builder*/) {},
@@ -56,15 +59,26 @@ class ApproxDistinctTest : public AggregationTestBase {
         {},
         {fmt::format("approx_set(c0, {})", maxStandardError)},
         {"cardinality(a0)"},
-        {expected});
+        {expected},
+        {},
+        testWithTableScan);
   }
 
-  void testGlobalAgg(const VectorPtr& values, int64_t expectedResult) {
+  void testGlobalAgg(
+      const VectorPtr& values,
+      int64_t expectedResult,
+      bool testWithTableScan = true) {
     auto vectors = makeRowVector({values});
     auto expected =
         makeRowVector({makeNullableFlatVector<int64_t>({expectedResult})});
 
-    testAggregations({vectors}, {}, {"approx_distinct(c0)"}, {expected});
+    testAggregations(
+        {vectors},
+        {},
+        {"approx_distinct(c0)"},
+        {expected},
+        {},
+        testWithTableScan);
     testAggregationsWithCompanion(
         {vectors},
         [](auto& /*builder*/) {},
@@ -75,7 +89,13 @@ class ApproxDistinctTest : public AggregationTestBase {
         {expected});
 
     testAggregations(
-        {vectors}, {}, {"approx_set(c0)"}, {"cardinality(a0)"}, {expected});
+        {vectors},
+        {},
+        {"approx_set(c0)"},
+        {"cardinality(a0)"},
+        {expected},
+        {},
+        testWithTableScan);
   }
 
   template <typename T, typename U>
@@ -304,6 +324,19 @@ TEST_F(ApproxDistinctTest, globalAggAllNulls) {
   EXPECT_TRUE(readSingleValue(op).isNull());
 }
 
+TEST_F(ApproxDistinctTest, hugeInt) {
+  auto hugeIntValues =
+      makeFlatVector<int128_t>(50000, [](auto row) { return row; });
+  // Last param is set false to disable tablescan test
+  // as DWRF writer doesn't have hugeint support.
+  // Refer:https://github.com/facebookincubator/velox/issues/7775
+  testGlobalAgg(hugeIntValues, 49669, false);
+  testGlobalAgg(
+      hugeIntValues, common::hll::kLowestMaxStandardError, 50110, false);
+  testGlobalAgg(
+      hugeIntValues, common::hll::kHighestMaxStandardError, 41741, false);
+}
+
 TEST_F(ApproxDistinctTest, streaming) {
   auto rawInput1 = makeFlatVector<int64_t>({1, 2, 3});
   auto rawInput2 = makeFlatVector<int64_t>(1000, folly::identity);