fix Nan & Infinity comparison And Value's operator/ & operator% (#4893)

vesoft-inc · Nov 17, 2022 · 618d7ed · 618d7ed
1 parent aaa0431
commit 618d7ed
Show file tree

Hide file tree

Showing 12 changed files with 211 additions and 79 deletions.
diff --git a/src/common/datatypes/Value.cpp b/src/common/datatypes/Value.cpp
@@ -2487,12 +2487,7 @@ Value operator/(const Value& lhs, const Value& rhs) {
           return lVal / denom;
         }
         case Value::Type::FLOAT: {
-          double denom = rhs.getFloat();
-          if (std::abs(denom) > kEpsilon) {
-            return lhs.getInt() / denom;
-          } else {
-            return Value::kNullDivByZero;
-          }
+          return lhs.getInt() / rhs.getFloat();
         }
         default: {
           return Value::kNullBadType;
@@ -2502,20 +2497,10 @@ Value operator/(const Value& lhs, const Value& rhs) {
     case Value::Type::FLOAT: {
       switch (rhs.type()) {
         case Value::Type::INT: {
-          int64_t denom = rhs.getInt();
-          if (denom != 0) {
-            return lhs.getFloat() / denom;
-          } else {
-            return Value::kNullDivByZero;
-          }
+          return lhs.getFloat() / rhs.getInt();
         }
         case Value::Type::FLOAT: {
-          double denom = rhs.getFloat();
-          if (std::abs(denom) > kEpsilon) {
-            return lhs.getFloat() / denom;
-          } else {
-            return Value::kNullDivByZero;
-          }
+          return lhs.getFloat() / rhs.getFloat();
         }
         default: {
           return Value::kNullBadType;
@@ -2548,12 +2533,7 @@ Value operator%(const Value& lhs, const Value& rhs) {
           }
         }
         case Value::Type::FLOAT: {
-          double denom = rhs.getFloat();
-          if (std::abs(denom) > kEpsilon) {
-            return std::fmod(lhs.getInt(), denom);
-          } else {
-            return Value::kNullDivByZero;
-          }
+          return std::fmod(lhs.getInt(), rhs.getFloat());
         }
         default: {
           return Value::kNullBadType;
@@ -2563,20 +2543,10 @@ Value operator%(const Value& lhs, const Value& rhs) {
     case Value::Type::FLOAT: {
       switch (rhs.type()) {
         case Value::Type::INT: {
-          int64_t denom = rhs.getInt();
-          if (denom != 0) {
-            return std::fmod(lhs.getFloat(), denom);
-          } else {
-            return Value::kNullDivByZero;
-          }
+          return std::fmod(lhs.getFloat(), rhs.getInt());
         }
         case Value::Type::FLOAT: {
-          double denom = rhs.getFloat();
-          if (std::abs(denom) > kEpsilon) {
-            return std::fmod(lhs.getFloat(), denom);
-          } else {
-            return Value::kNullDivByZero;
-          }
+          return std::fmod(lhs.getFloat(), rhs.getFloat());
         }
         default: {
           return Value::kNullBadType;
@@ -2877,11 +2847,11 @@ bool operator>(const Value& lhs, const Value& rhs) {
 }
 
 bool operator<=(const Value& lhs, const Value& rhs) {
-  return !(rhs < lhs);
+  return lhs < rhs || lhs == rhs;
 }
 
 bool operator>=(const Value& lhs, const Value& rhs) {
-  return !(lhs < rhs);
+  return lhs > rhs || lhs == rhs;
 }
 
 Value operator&&(const Value& lhs, const Value& rhs) {

diff --git a/src/common/datatypes/Value.h b/src/common/datatypes/Value.h
@@ -367,8 +367,12 @@ struct Value {
   Value toInt() const;
   Value toSet() const;
 
+  // Expr use this function instead of operator<, because a Value compare to a Null
+  // return null instead of true or false
   Value lessThan(const Value& v) const;
 
+  // Expr use this function instead of operator==, because a Value compare to a Null
+  // return null instead of true or false
   Value equal(const Value& v) const;
 
   // Whether the value can be converted to bool implicitly

diff --git a/src/common/datatypes/test/ValueTest.cpp b/src/common/datatypes/test/ValueTest.cpp
@@ -6,6 +6,8 @@
 #include <robin_hood.h>
 #include <thrift/lib/cpp2/protocol/Serializer.h>
 
+#include <cmath>
+
 #include "common/base/Base.h"
 #include "common/datatypes/CommonCpp2Ops.h"
 #include "common/datatypes/DataSet.h"
@@ -598,7 +600,8 @@ TEST(Value, Arithmetics) {
     EXPECT_EQ((vFloat1.getFloat() / vFloat2.getFloat()), v.getFloat());
 
     v = vFloat1 / vZero;
-    EXPECT_EQ(Value::Type::NULLVALUE, v.type());
+    EXPECT_EQ(Value::Type::FLOAT, v.type());
+    EXPECT_EQ(std::numeric_limits<double>::infinity(), v.getFloat());
     v = vInt1 / vZero;
     EXPECT_EQ(Value::Type::NULLVALUE, v.type());
   }
@@ -629,7 +632,8 @@ TEST(Value, Arithmetics) {
     EXPECT_EQ(std::fmod(vFloat1.getFloat(), vFloat2.getFloat()), v.getFloat());
 
     v = vFloat1 % vZero;
-    EXPECT_EQ(Value::Type::NULLVALUE, v.type());
+    EXPECT_EQ(Value::Type::FLOAT, v.type());
+    EXPECT_TRUE(std::isnan(v.getFloat()));
     v = vInt1 % vZero;
     EXPECT_EQ(Value::Type::NULLVALUE, v.type());
   }
@@ -682,6 +686,11 @@ TEST(Value, Comparison) {
   Value vInt2(2);
   Value vFloat1(3.14);
   Value vFloat2(2.67);
+  Value vFloat3(-2.67);
+  Value vFloatNaN(0 / 0.0);
+  Value vFloatPositiveInfinity(1 / 0.0);
+  Value vFloatNegativeInfinity(-1 / 0.0);
+
   Value vStr1("Hello ");
   Value vStr2("World");
   Value vBool1(false);
@@ -811,6 +820,99 @@ TEST(Value, Comparison) {
     v = vFloat1 <= vFloat2;
     EXPECT_EQ(Value::Type::BOOL, v.type());
     EXPECT_EQ(false, v.getBool());
+
+    // NaN comparison
+    // https://en.wikipedia.org/wiki/NaN#Comparison_with_NaN
+    // Comparison between NaN and any floating-point value x (including NaN and ±Inf)
+    // Comparison NaN ≥ x  NaN ≤ x  NaN > x  NaN < x  NaN = x  NaN ≠ x
+    //     Result False    False    False    False    False    True
+    v = vFloatNaN >= vFloat1;
+    EXPECT_EQ(Value::Type::BOOL, v.type());
+    EXPECT_EQ(false, v.getBool());
+
+    v = vFloatNaN > vFloat1;
+    EXPECT_EQ(Value::Type::BOOL, v.type());
+    EXPECT_EQ(false, v.getBool());
+
+    v = vFloatNaN < vFloat1;
+    EXPECT_EQ(Value::Type::BOOL, v.type());
+    EXPECT_EQ(false, v.getBool());
+
+    v = vFloatNaN.lessThan(vFloat1);
+    EXPECT_EQ(Value::Type::BOOL, v.type());
+    EXPECT_EQ(false, v.getBool());
+
+    v = vFloatNaN <= vFloat1;
+    EXPECT_EQ(Value::Type::BOOL, v.type());
+    EXPECT_EQ(false, v.getBool());
+
+    v = vFloatNaN >= vFloat3;
+    EXPECT_EQ(Value::Type::BOOL, v.type());
+    EXPECT_EQ(false, v.getBool());
+
+    v = vFloatNaN > vFloat3;
+    EXPECT_EQ(Value::Type::BOOL, v.type());
+    EXPECT_EQ(false, v.getBool());
+
+    v = vFloatNaN < vFloat3;
+    EXPECT_EQ(Value::Type::BOOL, v.type());
+    EXPECT_EQ(false, v.getBool());
+
+    v = vFloatNaN <= vFloat3;
+    EXPECT_EQ(Value::Type::BOOL, v.type());
+    EXPECT_EQ(false, v.getBool());
+
+    // NaN != any Value
+    v = vFloatNaN != vFloat3;
+    EXPECT_EQ(Value::Type::BOOL, v.type());
+    EXPECT_EQ(true, v.getBool());
+    v = vFloatNaN == vFloat3;
+    EXPECT_EQ(Value::Type::BOOL, v.type());
+    EXPECT_EQ(false, v.getBool());
+    v = vFloatNaN.equal(vFloat3);
+    EXPECT_EQ(Value::Type::BOOL, v.type());
+    EXPECT_EQ(false, v.getBool());
+
+    v = vFloatNaN.equal(Value(0 / 0.0));
+    EXPECT_EQ(Value::Type::BOOL, v.type());
+    EXPECT_EQ(false, v.getBool());
+  }
+
+  {
+    // -Inf
+    Value v = vFloatPositiveInfinity.lessThan(vFloatNegativeInfinity);
+    EXPECT_EQ(Value::Type::BOOL, v.type());
+    EXPECT_EQ(false, v.getBool());
+
+    v = vFloatNegativeInfinity.lessThan(vFloatPositiveInfinity);
+    EXPECT_EQ(Value::Type::BOOL, v.type());
+    EXPECT_EQ(true, v.getBool());
+
+    v = vFloatNegativeInfinity.lessThan(vInt1);
+    EXPECT_EQ(Value::Type::BOOL, v.type());
+    EXPECT_EQ(true, v.getBool());
+
+    v = vFloatNegativeInfinity.lessThan(vFloat1);
+    EXPECT_EQ(Value::Type::BOOL, v.type());
+    EXPECT_EQ(true, v.getBool());
+
+    // +Inf
+    v = vFloatPositiveInfinity.lessThan(vInt1);
+    EXPECT_EQ(Value::Type::BOOL, v.type());
+    EXPECT_EQ(false, v.getBool());
+
+    v = vFloatPositiveInfinity.lessThan(vFloat1);
+    EXPECT_EQ(Value::Type::BOOL, v.type());
+    EXPECT_EQ(false, v.getBool());
+
+    // NaN comparison always false
+    v = vFloatNegativeInfinity.lessThan(vFloatNaN);
+    EXPECT_EQ(Value::Type::BOOL, v.type());
+    EXPECT_EQ(false, v.getBool());
+
+    v = vFloatPositiveInfinity.lessThan(vFloatNaN);
+    EXPECT_EQ(Value::Type::BOOL, v.type());
+    EXPECT_EQ(false, v.getBool());
   }
 
   // int and float

diff --git a/src/common/expression/RelationalExpression.cpp b/src/common/expression/RelationalExpression.cpp
@@ -29,10 +29,10 @@ const Value& RelationalExpression::eval(ExpressionContext& ctx) {
       result_ = lhs.lessThan(rhs) || lhs.equal(rhs);
       break;
     case Kind::kRelGT:
-      result_ = !lhs.lessThan(rhs) && !lhs.equal(rhs);
+      result_ = rhs.lessThan(lhs);
       break;
     case Kind::kRelGE:
-      result_ = !lhs.lessThan(rhs) || lhs.equal(rhs);
+      result_ = rhs.lessThan(lhs) || lhs.equal(rhs);
       break;
     case Kind::kRelREG: {
       if (lhs.isBadNull() || rhs.isBadNull()) {

diff --git a/src/common/expression/test/ArithmeticExpressionTest.cpp b/src/common/expression/test/ArithmeticExpressionTest.cpp
@@ -22,13 +22,13 @@ TEST_F(ArithmeticExpressionTest, TestArithmeticExpression) {
     TEST_EXPR(11 * 2, 22);
     TEST_EXPR(11 * 2.2, 24.2);
     TEST_EXPR(100.4 / 4, 25.1);
-    TEST_EXPR(10.4 % 0, NullType::DIV_BY_ZERO);
-    TEST_EXPR(10 % 0.0, NullType::DIV_BY_ZERO);
-    TEST_EXPR(10.4 % 0.0, NullType::DIV_BY_ZERO);
+    TEST_EXPR(10.4 % 0, Value(std::numeric_limits<double>::quiet_NaN()));
+    TEST_EXPR(10 % 0.0, Value(std::numeric_limits<double>::quiet_NaN()));
+    TEST_EXPR(10.4 % 0.0, Value(std::numeric_limits<double>::quiet_NaN()));
     TEST_EXPR(10 / 0, NullType::DIV_BY_ZERO);
-    TEST_EXPR(12 / 0.0, NullType::DIV_BY_ZERO);
-    TEST_EXPR(187. / 0.0, NullType::DIV_BY_ZERO);
-    TEST_EXPR(17. / 0, NullType::DIV_BY_ZERO);
+    TEST_EXPR(12 / 0.0, std::numeric_limits<double>::infinity());
+    TEST_EXPR(187. / 0.0, std::numeric_limits<double>::infinity());
+    TEST_EXPR(17. / 0, std::numeric_limits<double>::infinity());
   }
   {
     TEST_EXPR(1 + 2 + 3.2, 6.2);

diff --git a/src/common/expression/test/TestBase.h b/src/common/expression/test/TestBase.h
@@ -77,7 +77,12 @@ class ExpressionTest : public ::testing::Test {
     Expression *ep = yieldSentence->yield()->yields()->back()->expr();
     auto eval = Expression::eval(ep, gExpCtxt);
     EXPECT_EQ(eval.type(), expected.type()) << "type check failed: " << ep->toString();
-    EXPECT_EQ(eval, expected) << "check failed: " << ep->toString();
+    // NaN is not equals to NaN, check equals should use std::isnan()
+    if (expected.type() == Value::Type::FLOAT && std::isnan(expected.getFloat())) {
+      EXPECT_TRUE(std::isnan(eval.getFloat())) << "check failed: " << ep->toString();
+    } else {
+      EXPECT_EQ(eval, expected) << "check failed: " << ep->toString();
+    }
   }
 
   void testToString(const std::string &exprSymbol, const char *expected) {

diff --git a/src/common/function/FunctionManager.cpp b/src/common/function/FunctionManager.cpp
@@ -592,16 +592,10 @@ FunctionManager::FunctionManager() {
         }
         case Value::Type::INT: {
           auto val = args[0].get().getInt();
-          if (val < 0) {
-            return Value::kNullValue;
-          }
           return std::sqrt(val);
         }
         case Value::Type::FLOAT: {
           auto val = args[0].get().getFloat();
-          if (val < 0) {
-            return Value::kNullValue;
-          }
           return std::sqrt(val);
         }
         default: {

diff --git a/src/common/function/test/FunctionManagerTest.cpp b/src/common/function/test/FunctionManagerTest.cpp
@@ -255,6 +255,8 @@ TEST_F(FunctionManagerTest, functionCall) {
   {
     TEST_FUNCTION(sqrt, args_["int"], 2.0);
     TEST_FUNCTION(sqrt, args_["float"], std::sqrt(1.1));
+    TEST_FUNCTION(sqrt, {Value(-1)}, std::sqrt(-1));
+    TEST_FUNCTION(sqrt, {Value(0)}, std::sqrt(0));
   }
   {
     TEST_FUNCTION(cbrt, args_["int"], std::cbrt(4));