Removing StatusOr from places where it's useless.

- StatusOr<bool> Field::Compare -> bool Field::Compare
DorosMichal · May 23, 2023 · e3b389b · e3b389b
1 parent 4acaa7f
commit e3b389b
Show file tree

Hide file tree

Showing 9 changed files with 37 additions and 50 deletions.
diff --git a/e2e_tests/run_tests.py b/e2e_tests/run_tests.py
@@ -21,18 +21,26 @@ def main():
     db1 = SQLiteDatabase()
     db2 = KomfyDatabase()
 
-    try:
-        test_runner = TestRunner(db1, db2, args.verbose)
-        test_runner.prepare(DATABASE_SCRIPT)
-        test_runner.run_tests(
-            [
-                "SELECT * FROM table1;\n",
-                "SELECT * FROM table1 t1, table1 t2 WHERE t1.a > t2.a\n",
-                "SELECT * FROM table1 t1, table1 t2, table1 t3 WHERE t1.pk > t2.pk AND t2.pk > t3.pk\n",
-            ]
-        )
-    except Exception:
-        exit(1)
+    """ try: """
+    test_runner = TestRunner(db1, db2, args.verbose)
+    test_runner.prepare(DATABASE_SCRIPT)
+    test_runner.run_tests(
+        [
+            "SELECT * FROM table1;\n",
+            "SELECT * FROM table1 t1, table1 t2 WHERE t1.a > t2.a;\n",
+            "SELECT * FROM table1 t1, table1 t2, table1 t3 WHERE t1.pk > t2.pk AND t2.pk > t3.pk;\n",
+            "SELECT * FROM table1;\n",
+            "SELECT b, a FROM table1;\n",
+            "SELECT * FROM table1 t1, table2 t2 WHERE t1.tab2_pk = t2.pk;\n",
+            "SELECT a, d FROM table1 t1, table2 t2 WHERE t1.tab2_pk = t2.pk;\n",
+            # "SELECT a, d FROM table1 t1, table2 t2 WHERE t1.tab2_pk = t2.pk ORDER BY d;\n",
+            "SELECT COUNT(a) FROM table1;\n",
+            # "SELECT d, MAX(a) FROM table1 t1, table2 t2 WHERE t1.tab2_pk = t2.pk GROUP BY d ORDER BY d DESC;\n",
+            "SELECT * FROM table1 WHERE a > 2;\n",
+        ]
+    )
+    """ except Exception: """
+    """     exit(1) """
 
 
 if __name__ == "__main__":

diff --git a/komfydb/common/field.h b/komfydb/common/field.h
@@ -25,7 +25,7 @@ class Field {
  public:
   virtual ~Field(){};
 
-  virtual absl::StatusOr<bool> Compare(const Op& op, const Field* f) const = 0;
+  virtual bool Compare(const Op& op, const Field* f) const = 0;
 
   virtual Type GetType() const = 0;
 

diff --git a/komfydb/common/int_field.cc b/komfydb/common/int_field.cc
@@ -16,11 +16,8 @@ void IntField::SetValue(int i) {
   value = i;
 }
 
-absl::StatusOr<bool> IntField::Compare(const Op& op, const Field* f) const {
-  if (f->GetType() != GetType()) {
-    return absl::InvalidArgumentError("Can't compare fields of different type");
-  }
-
+bool IntField::Compare(const Op& op, const Field* f) const {
+  assert(f->GetType() == GetType());
   int fv = static_cast<const IntField*>(f)->GetValue();
 
   switch (op.value) {
@@ -37,8 +34,6 @@ absl::StatusOr<bool> IntField::Compare(const Op& op, const Field* f) const {
       return value < fv;
     case Op::LESS_THAN_OR_EQ:
       return value <= fv;
-    default:
-      return absl::InvalidArgumentError("Unknown operator value");
   }
 }
 

diff --git a/komfydb/common/int_field.h b/komfydb/common/int_field.h
@@ -18,7 +18,7 @@ class IntField : public Field {
 
   ~IntField() override {}
 
-  absl::StatusOr<bool> Compare(const Op& op, const Field* f) const override;
+  bool Compare(const Op& op, const Field* f) const override;
 
   Type GetType() const override;
 

diff --git a/komfydb/common/string_field.cc b/komfydb/common/string_field.cc
@@ -26,10 +26,8 @@ void StringField::SetValue(absl::string_view s) {
   value = s;
 }
 
-absl::StatusOr<bool> StringField::Compare(const Op& op, const Field* f) const {
-  if (f->GetType() != GetType()) {
-    return absl::InvalidArgumentError("Can't compare fields of different type");
-  }
+bool StringField::Compare(const Op& op, const Field* f) const {
+  assert(f->GetType() == GetType());
 
   std::string fs = static_cast<const StringField*>(f)->GetValue();
   int cmp_val = value.compare(fs);
@@ -49,8 +47,6 @@ absl::StatusOr<bool> StringField::Compare(const Op& op, const Field* f) const {
       return cmp_val <= 0;
     case Op::LIKE:
       return value.find(fs, 0) != std::string::npos;
-    default:
-      return absl::InvalidArgumentError("Unknown operator value");
   }
 }
 

diff --git a/komfydb/common/string_field.h b/komfydb/common/string_field.h
@@ -23,7 +23,7 @@ class StringField : public Field {
 
   ~StringField() override {}
 
-  absl::StatusOr<bool> Compare(const Op& op, const Field* f) const override;
+  bool Compare(const Op& op, const Field* f) const override;
 
   std::string GetValue() const;
 

diff --git a/komfydb/execution/aggregate_tuple.cc b/komfydb/execution/aggregate_tuple.cc
@@ -38,23 +38,21 @@ absl::Status AggregateTuple::ApplyAggregate(AggregateType aggregate_type, int i,
       IntField* old_int_field = static_cast<IntField*>(fields[i].get());
       IntField* new_int_field = static_cast<IntField*>(new_field);
       int old_value = old_int_field->GetValue();
+      int new_value = new_int_field->GetValue();
       switch (aggregate_type) {
         case AggregateType::NONE: {
           break;
         }
         case AggregateType::AVG:
         case AggregateType::SUM: {
-          int new_value = new_int_field->GetValue();
           old_int_field->SetValue(old_value + new_value);
           break;
         }
         case AggregateType::MAX: {
-          int new_value = new_int_field->GetValue();
           old_int_field->SetValue(std::max(old_value, new_value));
           break;
         }
         case AggregateType::MIN: {
-          int new_value = new_int_field->GetValue();
           old_int_field->SetValue(std::min(old_value, new_value));
           break;
         }
@@ -73,22 +71,12 @@ absl::Status AggregateTuple::ApplyAggregate(AggregateType aggregate_type, int i,
         case AggregateType::NONE: {
           break;
         }
+        case AggregateType::MIN:
         case AggregateType::MAX: {
-          std::string new_value = new_string_field->GetValue();
-          ASSIGN_OR_RETURN(bool replace, old_string_field->Compare(
-                                             Op::LESS_THAN, new_string_field));
-          if (replace) {
-            old_string_field->SetValue(new_value);
-          }
-          break;
-        }
-        case AggregateType::MIN: {
-          std::string new_value = new_string_field->GetValue();
-          ASSIGN_OR_RETURN(
-              bool replace,
-              old_string_field->Compare(Op::GREATER_THAN, new_string_field));
-          if (replace) {
-            old_string_field->SetValue(new_value);
+          Op op = aggregate_type == AggregateType::MAX ? Op::LESS_THAN
+                                                       : Op::GREATER_THAN;
+          if (old_string_field->Compare(op, new_string_field)) {
+            old_string_field->SetValue(new_string_field->GetValue());
           }
           break;
         }

diff --git a/komfydb/execution/join_predicate.cc b/komfydb/execution/join_predicate.cc
@@ -12,7 +12,7 @@ bool JoinPredicate::Filter(Tuple const& l_tuple, Tuple const& r_tuple) {
   // we assume that if we got to this place all the values are correct
   Field* l_field = l_tuple.GetField(l_field_idx).value();
   Field* r_field = r_tuple.GetField(r_field_idx).value();
-  return l_field->Compare(op, r_field).value();
+  return l_field->Compare(op, r_field);
 }
 
 int JoinPredicate::GetField1idx() {

diff --git a/komfydb/execution/predicate.cc b/komfydb/execution/predicate.cc
@@ -46,11 +46,11 @@ bool Predicate::Evaluate(const Record& record) {
   /* We assume that GetField() or Compare() cannot fail here */
   switch (type) {
     case Type::COL_COL: {
-      return *(
-          (*record.GetField(l_field))->Compare(op, *record.GetField(r_field)));
+      return (*record.GetField(l_field))
+          ->Compare(op, *record.GetField(r_field));
     }
     case Type::COL_CONST: {
-      return *((*record.GetField(l_field))->Compare(op, const_field.get()));
+      return (*record.GetField(l_field))->Compare(op, const_field.get());
     }
   }
 }