Cleaning up Min/Max for TimestampType and VarlenType

cmu-db · Oct 21, 2017 · b2c95b4 · b2c95b4
1 parent ecfc1ed
commit b2c95b4
Show file tree

Hide file tree

Showing 4 changed files with 21 additions and 30 deletions.
diff --git a/src/include/type/timestamp_type.h b/src/include/type/timestamp_type.h
@@ -33,8 +33,8 @@ class TimestampType : public Type {
   CmpBool CompareGreaterThanEquals(const Value& left, const Value &right) const override;
 
   // Other mathematical functions
-  virtual Value Min(const Value& left, const Value &right) const override;
-  virtual Value Max(const Value& left, const Value &right) const override;
+  Value Min(const Value& left, const Value &right) const override;
+  Value Max(const Value& left, const Value &right) const override;
 
   bool IsInlined(const Value&) const override { return true; }
 

diff --git a/src/type/timestamp_type.cpp b/src/type/timestamp_type.cpp
@@ -67,20 +67,16 @@ CmpBool TimestampType::CompareGreaterThanEquals(const Value& left, const Value &
 
 Value TimestampType::Min(const Value& left, const Value& right) const {
   PL_ASSERT(left.CheckComparable(right));
-  if (left.IsNull() || right.IsNull())
-    return left.OperateNull(right);
-  if (left.CompareLessThan(right) == CMP_TRUE)
-      return left.Copy();
+  if (left.IsNull() || right.IsNull()) return left.OperateNull(right);
+  if (left.CompareLessThan(right) == CMP_TRUE) return left.Copy();
   return right.Copy();
 }
 
 Value TimestampType::Max(const Value& left, const Value& right) const {
-    PL_ASSERT(left.CheckComparable(right));
-    if (left.IsNull() || right.IsNull())
-        return left.OperateNull(right);
-    if (left.CompareGreaterThan(right) == CMP_TRUE)
-        return left.Copy();
-    return right.Copy();
+  PL_ASSERT(left.CheckComparable(right));
+  if (left.IsNull() || right.IsNull()) return left.OperateNull(right);
+  if (left.CompareGreaterThanEquals(right) == CMP_TRUE) return left.Copy();
+  return right.Copy();
 }
 
 // Debug

diff --git a/src/type/varlen_type.cpp b/src/type/varlen_type.cpp
@@ -129,21 +129,17 @@ CmpBool VarlenType::CompareGreaterThanEquals(const Value &left,
 }
 
 Value VarlenType::Min(const Value& left, const Value& right) const {
-    PL_ASSERT(left.CheckComparable(right));
-    if (left.IsNull() || right.IsNull())
-        return left.OperateNull(right);
-    if (left.CompareLessThan(right) == CMP_TRUE)
-        return left.Copy();
-    return right.Copy();
+  PL_ASSERT(left.CheckComparable(right));
+  if (left.IsNull() || right.IsNull()) return left.OperateNull(right);
+  if (left.CompareLessThan(right) == CMP_TRUE) return left.Copy();
+  return right.Copy();
 }
 
 Value VarlenType::Max(const Value& left, const Value& right) const {
-    PL_ASSERT(left.CheckComparable(right));
-    if (left.IsNull() || right.IsNull())
-        return left.OperateNull(right);
-    if (left.CompareGreaterThan(right) == CMP_TRUE)
-        return left.Copy();
-    return right.Copy();
+  PL_ASSERT(left.CheckComparable(right));
+  if (left.IsNull() || right.IsNull()) return left.OperateNull(right);
+  if (left.CompareGreaterThanEquals(right) == CMP_TRUE) return left.Copy();
+  return right.Copy();
 }
 
 std::string VarlenType::ToString(const Value &val) const {

diff --git a/test/type/value_test.cpp b/test/type/value_test.cpp
@@ -66,19 +66,18 @@ TEST_F(ValueTests, MinMaxTest) {
 
     // Special case for VARCHAR
     if (col_type == type::TypeId::VARCHAR) {
-      maxVal = type::ValueFactory::GetVarcharValue(std::string("A"), nullptr);
+      maxVal = type::ValueFactory::GetVarcharValue(std::string("AAA"), nullptr);
       minVal = type::ValueFactory::GetVarcharValue(std::string("ZZZ"), nullptr);
+      EXPECT_EQ(type::CMP_FALSE, minVal.CompareLessThan(maxVal));
+      EXPECT_EQ(type::CMP_FALSE, maxVal.CompareGreaterThan(minVal));
     }
 
-    LOG_TRACE("MinMax: %s", TypeIdToString(col_type).c_str());
-
     // FIXME: Broken types!!!
-    if (col_type == type::TypeId::BOOLEAN) continue;
     if (col_type == type::TypeId::VARCHAR) continue;
-    if (col_type == type::TypeId::TIMESTAMP) continue;
+    LOG_DEBUG("MinMax: %s", TypeIdToString(col_type).c_str());
 
     // Check that we always get the correct MIN value
-    EXPECT_EQ(type::CMP_TRUE, maxVal.Min(minVal).CompareEquals(minVal));
+    EXPECT_EQ(type::CMP_TRUE, minVal.Min(minVal).CompareEquals(minVal));
     EXPECT_EQ(type::CMP_TRUE, minVal.Min(maxVal).CompareEquals(minVal));
     EXPECT_EQ(type::CMP_TRUE, minVal.Min(minVal).CompareEquals(minVal));