fix: Allow merging OR-ed bigint single value ranges (facebookincubator#15738)

hdikeman · facebook-github-bot · commit 1a77f08bf6d9 · 2025-12-10T13:34:29.000-08:00
Summary:

Since BIGINT ranges can sometimes be converted to values filters (i.e. if all the range filters are single value), sequential merging of several BIGINT ranges can cause a generic MultiRange filter to be created, which cannot be evaluated.

e.g. `(id = 100 OR id = 200 OR id = 300)`

is converted by HiveConnectorUtil::extractFiltersFromRemainingFilter in the following stages:
```
I1209 13:29:11.955765 1472505 [CPUThreadPool0] ExprToSubfieldFilter.cpp:584] tryMergeBigintRanges called with 2 disjuncts
I1209 13:29:11.955819 1472505 [CPUThreadPool0] ExprToSubfieldFilter.cpp:586]   Disjunct 0: BigintRange: [100, 100] no nulls
I1209 13:29:11.955826 1472505 [CPUThreadPool0] ExprToSubfieldFilter.cpp:586]   Disjunct 1: BigintRange: [200, 200] no nulls
I1209 13:29:11.955830 1472505 [CPUThreadPool0] ExprToSubfieldFilter.cpp:602] Creating BigintValues with 2 values
I1209 13:29:11.955852 1472505 [CPUThreadPool0] ExprToSubfieldFilter.cpp:584] tryMergeBigintRanges called with 2 disjuncts
I1209 13:29:11.955862 1472505 [CPUThreadPool0] ExprToSubfieldFilter.cpp:586]   Disjunct 0: Filter(BigintValuesUsingBitmask, deterministic, no nulls)
I1209 13:29:11.955871 1472505 [CPUThreadPool0] ExprToSubfieldFilter.cpp:586]   Disjunct 1: BigintRange: [300, 300] no nulls
W1209 13:29:11.955875 1472505 [CPUThreadPool0] ExprToSubfieldFilter.cpp:609] tryMergeBigintRanges returning nullptr - not all disjuncts are BigintRange/BigintMultiRange
```

The filter is instead merged into a generic MultiRange filter with bigint ranges, which cannot be evaluated:
```
Filter(MultiRange, deterministic, no nulls): testInt64Range() is not supported.
```
Generic MultiRange does not support BIGINT type by design. Instead of converting to a MultiRange, merge logic should have explicit handling for BIGINT value filters so it can handle these gracefully

Differential Revision: D88789308
diff --git a/velox/connectors/hive/tests/HiveConnectorTest.cpp b/velox/connectors/hive/tests/HiveConnectorTest.cpp
@@ -739,6 +739,23 @@ TEST_F(HiveConnectorTest, disjuncts) {
     ASSERT_EQ(filters.begin()->first, Subfield("c0"));
     VELOX_ASSERT_FILTER(exec::between(1, 12), filters.begin()->second);
   }
+
+  {
+    auto expr = parseExpr("c0 not in (1, 3) or c0 in (1, 2)", rowType);
+    SubfieldFilters filters;
+    double sampleRate = 1;
+    auto remaining = extractFiltersFromRemainingFilter(
+        expr, &evaluator, filters, sampleRate);
+    ASSERT_EQ(sampleRate, 1);
+    ASSERT_FALSE(remaining);
+    ASSERT_EQ(filters.size(), 1);
+    VELOX_ASSERT_FILTER(
+        exec::bigintOr(
+            exec::between(
+                std::numeric_limits<int64_t>::min(), static_cast<int64_t>(2)),
+            exec::greaterThanOrEqual(static_cast<int64_t>(4))),
+        filters.begin()->second);
+  }
 }
 
 #undef VELOX_ASSERT_FILTER
diff --git a/velox/expression/ExprToSubfieldFilter.cpp b/velox/expression/ExprToSubfieldFilter.cpp
@@ -58,6 +58,68 @@ bool isBigintMultiRange(const std::unique_ptr<common::Filter>& filter) {
   return filter->is(common::FilterKind::kBigintMultiRange);
 }
 
+bool isBigintValues(const std::unique_ptr<common::Filter>& filter) {
+  return filter->is(common::FilterKind::kBigintValuesUsingHashTable) ||
+      filter->is(common::FilterKind::kBigintValuesUsingBitmask);
+}
+
+bool isNegatedBigintRange(const std::unique_ptr<common::Filter>& filter) {
+  return filter->is(common::FilterKind::kNegatedBigintRange);
+}
+
+bool isNegatedBigintValues(const std::unique_ptr<common::Filter>& filter) {
+  return filter->is(common::FilterKind::kNegatedBigintValuesUsingHashTable) ||
+      filter->is(common::FilterKind::kNegatedBigintValuesUsingBitmask);
+}
+
+std::vector<std::unique_ptr<common::BigintRange>> invertNegatedBigintRange(
+    const common::NegatedBigintRange* negatedRange) {
+  std::vector<std::unique_ptr<common::BigintRange>> ranges;
+  int64_t lower = negatedRange->lower();
+  int64_t upper = negatedRange->upper();
+
+  if (lower > std::numeric_limits<int64_t>::min()) {
+    ranges.emplace_back(
+        std::make_unique<common::BigintRange>(
+            std::numeric_limits<int64_t>::min(), lower - 1, false));
+  }
+  if (upper < std::numeric_limits<int64_t>::max()) {
+    ranges.emplace_back(
+        std::make_unique<common::BigintRange>(
+            upper + 1, std::numeric_limits<int64_t>::max(), false));
+  }
+  return ranges;
+}
+
+std::vector<std::unique_ptr<common::BigintRange>> invertNegatedBigintValues(
+    const std::vector<int64_t>& values) {
+  VELOX_CHECK(std::is_sorted(values.begin(), values.end()));
+  std::vector<std::unique_ptr<common::BigintRange>> ranges;
+  ranges.reserve(values.size() + 1);
+
+  if (values.front() > std::numeric_limits<int64_t>::min()) {
+    ranges.emplace_back(
+        std::make_unique<common::BigintRange>(
+            std::numeric_limits<int64_t>::min(), values.front() - 1, false));
+  }
+
+  for (size_t i = 0; i < values.size() - 1; ++i) {
+    if (values[i] + 1 <= values[i + 1] - 1) {
+      ranges.emplace_back(
+          std::make_unique<common::BigintRange>(
+              values[i] + 1, values[i + 1] - 1, false));
+    }
+  }
+
+  if (values.back() < std::numeric_limits<int64_t>::max()) {
+    ranges.emplace_back(
+        std::make_unique<common::BigintRange>(
+            values.back() + 1, std::numeric_limits<int64_t>::max(), false));
+  }
+
+  return ranges;
+}
+
 template <typename T, typename U>
 std::unique_ptr<T> asUniquePtr(std::unique_ptr<U> ptr) {
   return std::unique_ptr<T>(static_cast<T*>(ptr.release()));
@@ -597,25 +659,90 @@ std::unique_ptr<common::Filter> tryMergeBigintRanges(
   }
 
   if (!std::all_of(disjuncts.begin(), disjuncts.end(), [](const auto& filter) {
-        return isBigintRange(filter) || isBigintMultiRange(filter);
+        return isBigintRange(filter) || isBigintMultiRange(filter) ||
+            isBigintValues(filter) || isNegatedBigintRange(filter) ||
+            isNegatedBigintValues(filter);
       })) {
     return nullptr;
   }
 
   const bool nullAllowed = isNullAllowed(disjuncts);
 
+  std::vector<int64_t> values;
   std::vector<std::unique_ptr<common::BigintRange>> ranges;
+
   for (auto& filter : disjuncts) {
-    if (isBigintRange(filter)) {
-      ranges.emplace_back(asBigintRange(filter));
-    } else {
-      for (const auto& range :
-           filter->as<common::BigintMultiRange>()->ranges()) {
-        ranges.emplace_back(std::make_unique<common::BigintRange>(*range));
+    switch (filter->kind()) {
+      case common::FilterKind::kBigintRange: {
+        auto* range = filter->as<common::BigintRange>();
+        if (range->isSingleValue()) {
+          values.push_back(range->lower());
+        } else {
+          ranges.emplace_back(asBigintRange(filter));
+        }
+        break;
+      }
+      case common::FilterKind::kBigintMultiRange: {
+        for (const auto& range :
+             filter->as<common::BigintMultiRange>()->ranges()) {
+          if (range->isSingleValue()) {
+            values.push_back(range->lower());
+          } else {
+            ranges.emplace_back(std::make_unique<common::BigintRange>(*range));
+          }
+        }
+        break;
+      }
+      case common::FilterKind::kBigintValuesUsingHashTable:
+      case common::FilterKind::kBigintValuesUsingBitmask: {
+        std::vector<int64_t> vals;
+        if (filter->is(common::FilterKind::kBigintValuesUsingHashTable)) {
+          vals = filter->as<common::BigintValuesUsingHashTable>()->values();
+        } else {
+          vals = filter->as<common::BigintValuesUsingBitmask>()->values();
+        }
+        values.insert(values.end(), vals.begin(), vals.end());
+        break;
+      }
+      case common::FilterKind::kNegatedBigintRange: {
+        auto* negatedRange = filter->as<common::NegatedBigintRange>();
+        auto convertedRanges = invertNegatedBigintRange(negatedRange);
+        for (auto& range : convertedRanges) {
+          ranges.emplace_back(std::move(range));
+        }
+        break;
+      }
+      case common::FilterKind::kNegatedBigintValuesUsingHashTable:
+      case common::FilterKind::kNegatedBigintValuesUsingBitmask: {
+        std::vector<int64_t> vals;
+        if (filter->is(
+                common::FilterKind::kNegatedBigintValuesUsingHashTable)) {
+          vals =
+              filter->as<common::NegatedBigintValuesUsingHashTable>()->values();
+        } else {
+          vals =
+              filter->as<common::NegatedBigintValuesUsingBitmask>()->values();
+        }
+        auto convertedRanges = invertNegatedBigintValues(vals);
+        for (auto& range : convertedRanges) {
+          ranges.emplace_back(std::move(range));
+        }
+        break;
       }
+      default:
+        VELOX_UNREACHABLE();
     }
   }
 
+  if (ranges.empty()) {
+    return common::createBigintValues(values, nullAllowed);
+  }
+
+  for (int64_t value : values) {
+    ranges.emplace_back(
+        std::make_unique<common::BigintRange>(value, value, nullAllowed));
+  }
+
   std::sort(ranges.begin(), ranges.end(), [](const auto& a, const auto& b) {
     return a->lower() < b->lower();
   });
diff --git a/velox/expression/tests/ExprToSubfieldFilterTest.cpp b/velox/expression/tests/ExprToSubfieldFilterTest.cpp
@@ -283,6 +283,189 @@ TEST_F(ExprToSubfieldFilterTest, makeOrFilterBigint) {
   }
 }
 
+TEST_F(ExprToSubfieldFilterTest, makeOrFilterBigintValues) {
+  // (a in (100)) + (a in (200)) -> a in (100, 200)
+  {
+    auto values1 = in({100});
+    auto values2 = in({200});
+    auto expected = in({100, 200});
+
+    VELOX_ASSERT_FILTER(expected, makeOr(values1->clone(), values2->clone()));
+  }
+
+  // (a in (100)) + (a = 200) -> a in (100, 200)
+  {
+    auto values = in({100});
+    auto singleValueRange = equal(200);
+    auto expected = in({100, 200});
+
+    VELOX_ASSERT_FILTER(
+        expected, makeOr(values->clone(), singleValueRange->clone()));
+  }
+
+  // (a in (5)) + (a between 10 and 20) -> a = 5 or a between 10 and 20
+  {
+    auto values = in({5});
+    auto multiValueRange = between(10, 20);
+    auto expected = bigintOr(equal(5), between(10, 20));
+
+    VELOX_ASSERT_FILTER(
+        expected, makeOr(values->clone(), multiValueRange->clone()));
+  }
+
+  // (a = 100 or a = 200) + (a = 300) -> a in (100, 200, 300)
+  {
+    std::vector<std::unique_ptr<common::BigintRange>> ranges;
+    ranges.emplace_back(std::make_unique<common::BigintRange>(100, 100, false));
+    ranges.emplace_back(std::make_unique<common::BigintRange>(200, 200, false));
+    auto multiRangeAllSingle =
+        std::make_unique<common::BigintMultiRange>(std::move(ranges), false);
+
+    auto singleValueRange = equal(300);
+    auto expected = in({100, 200, 300});
+
+    VELOX_ASSERT_FILTER(
+        expected,
+        makeOr(singleValueRange->clone(), multiRangeAllSingle->clone()));
+  }
+
+  // (a = 5 or a between 10 and 20) + (a = 100) ->
+  //     (a = 5 or a between 10 and 20 or a = 100)
+  {
+    std::vector<std::unique_ptr<common::BigintRange>> ranges;
+    ranges.emplace_back(std::make_unique<common::BigintRange>(5, 5, false));
+    ranges.emplace_back(std::make_unique<common::BigintRange>(10, 20, false));
+    auto multiRangeMixed =
+        std::make_unique<common::BigintMultiRange>(std::move(ranges), false);
+
+    auto singleValueRange = equal(100);
+
+    std::vector<std::unique_ptr<common::BigintRange>> expectedRanges;
+    expectedRanges.emplace_back(
+        std::make_unique<common::BigintRange>(5, 5, false));
+    expectedRanges.emplace_back(
+        std::make_unique<common::BigintRange>(10, 20, false));
+    expectedRanges.emplace_back(
+        std::make_unique<common::BigintRange>(100, 100, false));
+    auto expected = std::make_unique<common::BigintMultiRange>(
+        std::move(expectedRanges), false);
+
+    VELOX_ASSERT_FILTER(
+        expected, makeOr(singleValueRange->clone(), multiRangeMixed->clone()));
+  }
+
+  // (a = 5 or a = 15) + (a between 10 and 20) -> (a = 5 or a between 10 and 20)
+  {
+    std::vector<std::unique_ptr<common::BigintRange>> ranges;
+    ranges.emplace_back(std::make_unique<common::BigintRange>(5, 5, false));
+    ranges.emplace_back(std::make_unique<common::BigintRange>(15, 15, false));
+    auto multiRangeAllSingle =
+        std::make_unique<common::BigintMultiRange>(std::move(ranges), false);
+
+    auto multiValueRange = between(10, 20);
+
+    std::vector<std::unique_ptr<common::BigintRange>> expectedRanges;
+    expectedRanges.emplace_back(
+        std::make_unique<common::BigintRange>(5, 5, false));
+    expectedRanges.emplace_back(
+        std::make_unique<common::BigintRange>(10, 20, false));
+    auto expected = std::make_unique<common::BigintMultiRange>(
+        std::move(expectedRanges), false);
+
+    VELOX_ASSERT_FILTER(
+        expected,
+        makeOr(multiValueRange->clone(), multiRangeAllSingle->clone()));
+  }
+
+  // (a = 5 or a between 25 and 30) + (a between 10 and 20) ->
+  //      (a = 5 or a between 10 and 20 or a between 25 and 30)
+  {
+    std::vector<std::unique_ptr<common::BigintRange>> ranges;
+    ranges.emplace_back(std::make_unique<common::BigintRange>(5, 5, false));
+    ranges.emplace_back(std::make_unique<common::BigintRange>(25, 30, false));
+    auto multiRangeMixed =
+        std::make_unique<common::BigintMultiRange>(std::move(ranges), false);
+
+    auto multiValueRange = between(10, 20);
+
+    std::vector<std::unique_ptr<common::BigintRange>> expectedRanges;
+    expectedRanges.emplace_back(
+        std::make_unique<common::BigintRange>(5, 5, false));
+    expectedRanges.emplace_back(
+        std::make_unique<common::BigintRange>(10, 20, false));
+    expectedRanges.emplace_back(
+        std::make_unique<common::BigintRange>(25, 30, false));
+    auto expected = std::make_unique<common::BigintMultiRange>(
+        std::move(expectedRanges), false);
+
+    VELOX_ASSERT_FILTER(
+        expected, makeOr(multiValueRange->clone(), multiRangeMixed->clone()));
+  }
+
+  // (a not in (5)) + (a = 10) -> all values except 5
+  {
+    auto negatedValues = notIn({5});
+    auto singleValue = equal(10);
+    auto expected = bigintOr(
+        between(std::numeric_limits<int64_t>::min(), 4), greaterThanOrEqual(6));
+
+    VELOX_ASSERT_FILTER(
+        expected, makeOr(negatedValues->clone(), singleValue->clone()));
+  }
+
+  // (a not in (1, 3)) + (a in (1, 2)) ->
+  //     (a between INT64_MIN and 2 or a between 3 and INT64_MAX)
+  {
+    auto negatedValues = notIn({1, 3});
+    auto values = in({1, 2});
+    auto expected = bigintOr(
+        between(std::numeric_limits<int64_t>::min(), 2), greaterThanOrEqual(4));
+
+    VELOX_ASSERT_FILTER(
+        expected, makeOr(negatedValues->clone(), values->clone()));
+  }
+
+  // (a != 10) + (a = 10) -> all non-null values
+  {
+    auto negatedRange = notEqual(10);
+    auto singleValue = equal(10);
+
+    VELOX_ASSERT_FILTER(
+        isNotNull(), makeOr(negatedRange->clone(), singleValue->clone()));
+  }
+
+  // (a not in (5, 10, 15)) + (a between 8 and 12) ->
+  //    [INT64_MIN, 4] or [6, 14] or [16, INT64_MAX]
+  {
+    auto negatedValues = notIn({5, 10, 15});
+    auto range = between(8, 12);
+
+    std::vector<std::unique_ptr<common::BigintRange>> expectedRanges;
+    expectedRanges.emplace_back(
+        std::make_unique<common::BigintRange>(
+            std::numeric_limits<int64_t>::min(), 4, false));
+    expectedRanges.emplace_back(
+        std::make_unique<common::BigintRange>(6, 14, false));
+    expectedRanges.emplace_back(
+        std::make_unique<common::BigintRange>(
+            16, std::numeric_limits<int64_t>::max(), false));
+    auto expected = std::make_unique<common::BigintMultiRange>(
+        std::move(expectedRanges), false);
+
+    VELOX_ASSERT_FILTER(
+        expected, makeOr(negatedValues->clone(), range->clone()));
+  }
+
+  // (a != 5) + (a != 10) -> all non-null values
+  {
+    auto negated1 = notEqual(5);
+    auto negated2 = notEqual(10);
+
+    VELOX_ASSERT_FILTER(
+        isNotNull(), makeOr(negated1->clone(), negated2->clone()));
+  }
+}
+
 TEST_F(ExprToSubfieldFilterTest, makeOrFilterDouble) {
   // a < 1.5 or a > 1.0 ==> not null
   {
