Remove StopAtRhsNull mode during comparison (facebookincubator#6444)

Summary: Pull Request resolved: facebookincubator#6444

Reviewed By: laithsakka

Differential Revision: D49020011

fbshipit-source-id: eecf25d1cc29c7356b2388016a27379f89dab2ca

velox/common/base/CompareFlags.h

@@ -26,9 +26,7 @@ struct CompareFlags {
   // NoStop: The compare doesn't stop at null.
   // StopAtNull: The compare returns std::nullopt if null is encountered in rhs
   // or lhs.
-  // StopAtRhsNull: The compare returns std::nullopt only if null encountered on
-  // the right hand side; return false, if it is on the left hand side.
-  enum class NullHandlingMode { NoStop, StopAtNull, StopAtRhsNull };
+  enum class NullHandlingMode { NoStop, StopAtNull };
 
   // This flag will be ignored if nullHandlingMode is true.
   bool nullsFirst = true;
@@ -41,8 +39,7 @@ struct CompareFlags {
   NullHandlingMode nullHandlingMode = NullHandlingMode::NoStop;
 
   bool mayStopAtNull() {
-    return nullHandlingMode == CompareFlags::NullHandlingMode::StopAtNull ||
-        nullHandlingMode == CompareFlags::NullHandlingMode::StopAtRhsNull;
+    return nullHandlingMode == CompareFlags::NullHandlingMode::StopAtNull;
   }
 
   static std::string nullHandlingModeToStr(NullHandlingMode mode) {
@@ -51,8 +48,6 @@ struct CompareFlags {
         return "NoStop";
       case CompareFlags::NullHandlingMode::StopAtNull:
         return "StopAtNull";
-      case CompareFlags::NullHandlingMode::StopAtRhsNull:
-        return "StopAtRhsNull";
       default:
         return fmt::format(
             "Unknown Null Handling mode {}", static_cast<int>(mode));

velox/expression/tests/GenericViewTest.cpp

@@ -146,11 +146,6 @@ TEST_F(GenericViewTest, compare) {
   flags.nullHandlingMode = CompareFlags::NullHandlingMode::StopAtNull;
   ASSERT_FALSE(reader[0].compare(reader[2], flags).has_value());
   ASSERT_TRUE(reader[0].compare(reader[1], flags).has_value());
-
-  flags.nullHandlingMode = CompareFlags::NullHandlingMode::StopAtRhsNull;
-  ASSERT_TRUE(reader[2].compare(reader[0], flags).has_value());
-  ASSERT_FALSE(reader[0].compare(reader[2], flags).has_value());
-  ASSERT_FALSE(reader[2].compare(reader[2], flags).has_value());
 }
 
 // Test reader<Generic> where generic elements are arrays<ints>

velox/vector/BaseVector.h

@@ -793,11 +793,6 @@ class BaseVector {
   compareNulls(bool thisNull, bool otherNull, CompareFlags flags) {
     DCHECK(thisNull || otherNull);
     switch (flags.nullHandlingMode) {
-      case CompareFlags::NullHandlingMode::StopAtRhsNull:
-        if (!otherNull) {
-          return false;
-        }
-        [[fallthrough]];
       case CompareFlags::NullHandlingMode::StopAtNull:
         return std::nullopt;
       case CompareFlags::NullHandlingMode::NoStop:

velox/vector/ComplexVector.cpp

@@ -616,10 +616,7 @@ std::optional<int32_t> compareArrays(
   for (auto i = 0; i < compareSize; ++i) {
     auto result =
         left.compare(&right, leftRange.begin + i, rightRange.begin + i, flags);
-    if ((flags.nullHandlingMode == CompareFlags::NullHandlingMode::StopAtNull ||
-         flags.nullHandlingMode ==
-             CompareFlags::NullHandlingMode::StopAtRhsNull) &&
-        !result.has_value()) {
+    if (flags.mayStopAtNull() && !result.has_value()) {
       // Null is encountered.
       return std::nullopt;
     }
@@ -644,10 +641,7 @@ std::optional<int32_t> compareArrays(
   auto compareSize = std::min(leftRange.size(), rightRange.size());
   for (auto i = 0; i < compareSize; ++i) {
     auto result = left.compare(&right, leftRange[i], rightRange[i], flags);
-    if ((flags.nullHandlingMode == CompareFlags::NullHandlingMode::StopAtNull ||
-         flags.nullHandlingMode ==
-             CompareFlags::NullHandlingMode::StopAtRhsNull) &&
-        !result.has_value()) {
+    if (flags.mayStopAtNull() && !result.has_value()) {
       // Null is encountered.
       return std::nullopt;
     }
@@ -886,10 +880,7 @@ std::optional<int32_t> MapVector::compare(
       compareArrays(*keys_, *otherMap->keys_, leftIndices, rightIndices, flags);
   VELOX_DCHECK(result.has_value(), "keys can not have null");
 
-  if ((flags.nullHandlingMode == CompareFlags::NullHandlingMode::StopAtNull ||
-       flags.nullHandlingMode ==
-           CompareFlags::NullHandlingMode::StopAtRhsNull) &&
-      !result.has_value()) {
+  if (flags.mayStopAtNull() && !result.has_value()) {
     return std::nullopt;
   }
 

velox/vector/tests/VectorCompareTest.cpp

@@ -28,19 +28,15 @@ class VectorCompareTest : public testing::Test,
   bool static constexpr kExpectNull = true;
   bool static constexpr kExpectNotNull = false;
   bool static constexpr kEqualsOnly = true;
-  bool static constexpr kStopAtNullRhs = true;
 
   void testCompareWithStopAtNull(
       const VectorPtr& vector,
       vector_size_t index1,
       vector_size_t index2,
       bool expectNull,
-      bool equalsOnly = false,
-      bool stopAtNullRhs = false) {
+      bool equalsOnly = false) {
     CompareFlags testFlags;
-    testFlags.nullHandlingMode = stopAtNullRhs
-        ? CompareFlags::NullHandlingMode::StopAtRhsNull
-        : CompareFlags::NullHandlingMode::StopAtNull;
+    testFlags.nullHandlingMode = CompareFlags::NullHandlingMode::StopAtNull;
     testFlags.equalsOnly = equalsOnly;
 
     ASSERT_EQ(
@@ -102,11 +98,9 @@ TEST_F(VectorCompareTest, compareStopAtNullArray) {
                   const std::optional<std::vector<std::optional<int64_t>>>&
                       array2,
                   bool expectNull,
-                  bool stopAtNullRhsOnly = false,
                   bool equalsOnly = false) {
     auto vector = vectorMaker_.arrayVectorNullable<int64_t>({array1, array2});
-    testCompareWithStopAtNull(
-        vector, 0, 1, expectNull, equalsOnly, stopAtNullRhsOnly);
+    testCompareWithStopAtNull(vector, 0, 1, expectNull, equalsOnly);
   };
 
   test(std::nullopt, std::nullopt, kExpectNull);
@@ -134,7 +128,6 @@ TEST_F(VectorCompareTest, compareStopAtNullArray) {
       {{std::nullopt, std::nullopt}},
       {{std::nullopt, std::nullopt, std::nullopt}},
       kExpectNotNull,
-      false,
       kEqualsOnly);
 
   // Since kEqualsOnly = false, the first two elements will be read.
@@ -147,19 +140,12 @@ TEST_F(VectorCompareTest, compareStopAtNullArray) {
       {{std::nullopt, std::nullopt}},
       {{std::nullopt, std::nullopt}},
       kExpectNull,
-      false,
       kEqualsOnly);
 
   test(
       {{std::nullopt, std::nullopt}},
       {{std::nullopt, std::nullopt}},
       kExpectNull);
-
-  // Stops if null is on the right hand side.
-  test({{1, std::nullopt}}, {{1, 1}}, kExpectNull);
-  test({{1, std::nullopt}}, {{1, 1}}, kExpectNotNull, kStopAtNullRhs);
-  test({{1, 1}}, {{1, std::nullopt}}, kExpectNull);
-  test({{1, 1}}, {{1, std::nullopt}}, kExpectNull, kStopAtNullRhs);
 }
 
 TEST_F(VectorCompareTest, compareStopAtNullMap) {
@@ -168,11 +154,9 @@ TEST_F(VectorCompareTest, compareStopAtNullMap) {
   auto test = [&](const map_t& map1,
                   const map_t& map2,
                   bool expectNull,
-                  bool stopAtNullRhsOnly = false,
                   bool equalsOnly = false) {
     auto vector = makeNullableMapVector<int64_t, int64_t>({map1, map2});
-    testCompareWithStopAtNull(
-        vector, 0, 1, expectNull, equalsOnly, stopAtNullRhsOnly);
+    testCompareWithStopAtNull(vector, 0, 1, expectNull, equalsOnly);
   };
 
   test({{{1, 2}, {3, 4}}}, {{{1, 2}, {3, 4}}}, kExpectNotNull);
@@ -207,22 +191,7 @@ TEST_F(VectorCompareTest, compareStopAtNullMap) {
       {{{1, 2}, {1, std::nullopt}}},
       {{{1, std::nullopt}}},
       kExpectNotNull,
-      false,
       kEqualsOnly);
-
-  // Stops if null is on the right hand side.
-  test({{{1, std::nullopt}, {3, 4}}}, {{{1, 2}, {3, 4}}}, kExpectNull);
-  test(
-      {{{1, std::nullopt}, {3, 4}}},
-      {{{1, 2}, {3, 4}}},
-      kExpectNotNull,
-      kStopAtNullRhs);
-  test({{{1, 2}, {3, 4}}}, {{{1, 2}, {3, std::nullopt}}}, kExpectNull);
-  test(
-      {{{1, 2}, {3, 4}}},
-      {{{1, 2}, {3, std::nullopt}}},
-      kExpectNull,
-      kStopAtNullRhs);
 }
 
 TEST_F(VectorCompareTest, compareStopAtNullRow) {
@@ -232,16 +201,14 @@ TEST_F(VectorCompareTest, compareStopAtNullRow) {
           const std::tuple<std::optional<int64_t>, std::optional<int64_t>>&
               row2,
           bool expectNull,
-          bool stopAtNullRhsOnly = false,
           bool equalsOnly = false) {
         auto vector = vectorMaker_.rowVector(
             {vectorMaker_.flatVectorNullable<int64_t>(
                  {std::get<0>(row1), std::get<0>(row2)}),
              vectorMaker_.flatVectorNullable<int64_t>(
                  {std::get<1>(row1), std::get<1>(row2)})});
 
-        testCompareWithStopAtNull(
-            vector, 0, 1, expectNull, equalsOnly, stopAtNullRhsOnly);
+        testCompareWithStopAtNull(vector, 0, 1, expectNull, equalsOnly);
       };
 
   test({1, 2}, {2, 3}, kExpectNotNull);
@@ -251,12 +218,6 @@ TEST_F(VectorCompareTest, compareStopAtNullRow) {
   test({1, 2}, {1, std::nullopt}, kExpectNull);
   test({1, std::nullopt}, {1, 2}, kExpectNull);
   test({1, 2}, {std::nullopt, 2}, kExpectNull);
-
-  // Stops if null is on the right hand side.
-  test({std::nullopt, 2}, {1, 2}, kExpectNull);
-  test({std::nullopt, 2}, {1, 2}, kExpectNotNull, kStopAtNullRhs);
-  test({1, 2}, {1, std::nullopt}, kExpectNull);
-  test({1, 2}, {1, std::nullopt}, kExpectNull, kStopAtNullRhs);
 }
 
 TEST_F(VectorCompareTest, CompareWithNullChildVector) {