Query::Plan::OrderBy

Reviewers: mislav.bradac, teon.banek

Reviewed By: teon.banek

Subscribers: pullbot

Differential Revision: https://phabricator.memgraph.io/D302

src/query/interpret/eval.hpp

@@ -95,8 +95,7 @@ class ExpressionEvaluator : public TreeVisitorBase {
 
   void PostVisit(IsNullOperator &) override {
     auto expression = PopBack();
-    result_stack_.push_back(
-        TypedValue(expression.type() == TypedValue::Type::Null));
+    result_stack_.push_back(TypedValue(expression.IsNull()));
   }
 
 #undef BINARY_OPERATOR_VISITOR

src/query/plan/operator.cpp

@@ -383,8 +383,7 @@ bool NodeFilter::NodeFilterCursor::VertexPasses(
     prop_pair.second->Accept(expression_evaluator);
     TypedValue comparison_result =
         vertex.PropsAt(prop_pair.first) == expression_evaluator.PopBack();
-    if (comparison_result.type() == TypedValue::Type::Null ||
-        !comparison_result.Value<bool>())
+    if (comparison_result.IsNull() || !comparison_result.Value<bool>())
       return false;
   }
   return true;
@@ -438,8 +437,7 @@ bool EdgeFilter::EdgeFilterCursor::EdgePasses(const EdgeAccessor &edge,
     prop_pair.second->Accept(expression_evaluator);
     TypedValue comparison_result =
         edge.PropsAt(prop_pair.first) == expression_evaluator.PopBack();
-    if (comparison_result.type() == TypedValue::Type::Null ||
-        !comparison_result.Value<bool>())
+    if (comparison_result.IsNull() || !comparison_result.Value<bool>())
       return false;
   }
   return true;
@@ -472,8 +470,7 @@ bool Filter::FilterCursor::Pull(Frame &frame, const SymbolTable &symbol_table) {
   while (input_cursor_->Pull(frame, symbol_table)) {
     self_.expression_->Accept(evaluator);
     TypedValue result = evaluator.PopBack();
-    if (result.type() == TypedValue::Type::Null || !result.Value<bool>())
-      continue;
+    if (result.IsNull() || !result.Value<bool>()) continue;
     return true;
   }
   return false;
@@ -1109,7 +1106,7 @@ void Aggregate::AggregateCursor::Update(
     TypedValue input_value = evaluator.PopBack();
 
     // Aggregations skip Null input values.
-    if (input_value.type() == TypedValue::Type::Null) continue;
+    if (input_value.IsNull()) continue;
 
     const auto &agg_op = std::get<1>(*agg_elem_it);
     *count_it += 1;
@@ -1260,7 +1257,6 @@ Limit::LimitCursor::LimitCursor(Limit &self, GraphDbAccessor &db)
     : self_(self), input_cursor_(self_.input_->MakeCursor(db)) {}
 
 bool Limit::LimitCursor::Pull(Frame &frame, const SymbolTable &symbol_table) {
-
   // we need to evaluate the limit expression before the first input Pull
   // because it might be 0 and thereby we shouldn't Pull from input at all
   // we can do this before Pulling from the input because the limit expression
@@ -1279,11 +1275,149 @@ bool Limit::LimitCursor::Pull(Frame &frame, const SymbolTable &symbol_table) {
   }
 
   // check we have not exceeded the limit before pulling
-  if (pulled_++ >= limit_)
-    return false;
+  if (pulled_++ >= limit_) return false;
 
   return input_cursor_->Pull(frame, symbol_table);
 }
 
+OrderBy::OrderBy(const std::shared_ptr<LogicalOperator> &input,
+                 const std::vector<std::pair<Ordering, Expression *>> order_by,
+                 const std::vector<Symbol> remember)
+    : input_(input), remember_(remember) {
+  // split the order_by vector into two vectors of orderings and expressions
+  std::vector<Ordering> ordering;
+  ordering.reserve(order_by.size());
+  order_by_.reserve(order_by.size());
+  for (const auto &ordering_expression_pair : order_by) {
+    ordering.emplace_back(ordering_expression_pair.first);
+    order_by_.emplace_back(ordering_expression_pair.second);
+  }
+  compare_ = TypedValueListCompare(ordering);
+}
+
+void OrderBy::Accept(LogicalOperatorVisitor &visitor) {
+  if (visitor.PreVisit(*this)) {
+    visitor.Visit(*this);
+    input_->Accept(visitor);
+    visitor.PostVisit(*this);
+  }
+}
+
+std::unique_ptr<Cursor> OrderBy::MakeCursor(GraphDbAccessor &db) {
+  return std::make_unique<OrderByCursor>(*this, db);
+}
+
+OrderBy::OrderByCursor::OrderByCursor(OrderBy &self, GraphDbAccessor &db)
+    : self_(self), input_cursor_(self_.input_->MakeCursor(db)) {}
+
+bool OrderBy::OrderByCursor::Pull(Frame &frame,
+                                  const SymbolTable &symbol_table) {
+  if (!did_pull_all_) {
+    ExpressionEvaluator evaluator(frame, symbol_table);
+    while (input_cursor_->Pull(frame, symbol_table)) {
+      // collect the order_by elements
+      std::list<TypedValue> order_by;
+      for (auto expression_ptr : self_.order_by_) {
+        expression_ptr->Accept(evaluator);
+        order_by.emplace_back(evaluator.PopBack());
+      }
+
+      // collect the remember elements
+      std::list<TypedValue> remember;
+      for (const Symbol &remember_sym : self_.remember_)
+        remember.emplace_back(frame[remember_sym]);
+
+      cache_.emplace_back(order_by, remember);
+    }
+
+    std::sort(cache_.begin(), cache_.end(),
+              [this](const auto &pair1, const auto &pair2) {
+                return self_.compare_(pair1.first, pair2.first);
+              });
+
+    did_pull_all_ = true;
+    cache_it_ = cache_.begin();
+  }
+
+  if (cache_it_ == cache_.end()) return false;
+
+  // place the remembered values on the frame
+  debug_assert(self_.remember_.size() == cache_it_->second.size(),
+               "Number of values does not match the number of remember symbols "
+               "in OrderBy");
+  auto remember_sym_it = self_.remember_.begin();
+  for (const TypedValue &remember : cache_it_->second)
+    frame[*remember_sym_it++] = remember;
+
+  cache_it_++;
+  return true;
+}
+
+bool OrderBy::TypedValueCompare(const TypedValue &a, const TypedValue &b) {
+  // in ordering null comes after everything else
+  // at the same time Null is not less that null
+  // first deal with Null < Whatever case
+  if (a.IsNull()) return false;
+  // now deal with NotNull < Null case
+  if (b.IsNull()) return true;
+
+  // comparisons are from this point legal only between values of
+  // the  same type, or int+float combinations
+  if ((a.type() != b.type() && !(a.IsNumeric() && b.IsNumeric())))
+    throw QueryRuntimeException(
+        "Can't compare value of type {} to value of type {}", a.type(),
+        b.type());
+
+  switch (a.type()) {
+    case TypedValue::Type::Bool:
+      return !a.Value<bool>() && b.Value<bool>();
+    case TypedValue::Type::Int:
+      if (b.type() == TypedValue::Type::Double)
+        return a.Value<int64_t>() < b.Value<double>();
+      else
+        return a.Value<int64_t>() < b.Value<int64_t>();
+    case TypedValue::Type::Double:
+      if (b.type() == TypedValue::Type::Int)
+        return a.Value<double>() < b.Value<int64_t>();
+      else
+        return a.Value<double>() < b.Value<double>();
+    case TypedValue::Type::String:
+      return a.Value<std::string>() < b.Value<std::string>();
+    case TypedValue::Type::List:
+    case TypedValue::Type::Map:
+    case TypedValue::Type::Vertex:
+    case TypedValue::Type::Edge:
+    case TypedValue::Type::Path:
+      throw QueryRuntimeException(
+          "Comparison is not defined for values of type {}", a.type());
+    default:
+      permanent_fail("Unhandled comparison for types");
+  }
+}
+
+bool OrderBy::TypedValueListCompare::operator()(
+    const std::list<TypedValue> &c1, const std::list<TypedValue> &c2) const {
+  auto c1_it = c1.begin();
+  auto c2_it = c2.begin();
+  // ordering is invalid if there are more elements in the collections
+  // then there are in the ordering_ vector
+  debug_assert(std::distance(c1_it, c1.end()) <= ordering_.size() &&
+                   std::distance(c2_it, c2.end()) <= ordering_.size(),
+               "Collections contain more elements then there are orderings");
+
+  auto ordering_it = ordering_.begin();
+  for (; c1_it != c1.end() && c2_it != c2.end();
+       c1_it++, c2_it++, ordering_it++) {
+    if (OrderBy::TypedValueCompare(*c1_it, *c2_it))
+      return *ordering_it == Ordering::ASC;
+    if (OrderBy::TypedValueCompare(*c2_it, *c1_it))
+      return *ordering_it == Ordering::DESC;
+  }
+
+  // at least one collection is exhausted
+  // c1 is less then c2 iff c1 reached the end but c2 didn't
+  return (c1_it == c1.end()) && (c2_it != c2.end());
+}
+
 }  // namespace plan
 }  // namespace query

src/query/plan/operator.hpp

@@ -4,8 +4,10 @@
 
 #include <algorithm>
 #include <memory>
+#include <query/exceptions.hpp>
 #include <tuple>
 #include <unordered_map>
+#include <utils/exceptions/not_yet_implemented.hpp>
 #include <vector>
 
 #include "database/graph_db_accessor.hpp"
@@ -63,6 +65,7 @@ class AdvanceCommand;
 class Aggregate;
 class Skip;
 class Limit;
+class OrderBy;
 
 /** @brief Base class for visitors of @c LogicalOperator class hierarchy. */
 using LogicalOperatorVisitor =
@@ -71,7 +74,7 @@ using LogicalOperatorVisitor =
                      SetProperties, SetLabels, RemoveProperty, RemoveLabels,
                      ExpandUniquenessFilter<VertexAccessor>,
                      ExpandUniquenessFilter<EdgeAccessor>, Accumulate,
-                     AdvanceCommand, Aggregate, Skip, Limit>;
+                     AdvanceCommand, Aggregate, Skip, Limit, OrderBy>;
 
 /** @brief Base class for logical operators.
  *
@@ -1008,5 +1011,72 @@ class Limit : public LogicalOperator {
   };
 };
 
+/** @brief Logical operator for ordering (sorting) results.
+ *
+ * Sorts the input rows based on an arbitrary number of
+ * Expressions. Ascending or descending ordering can be chosen
+ * for each independently (not providing enough orderings
+ * results in a runtime error).
+ *
+ * For each row an arbitrary number of Frame elements can be
+ * remembered. Only these elements (defined by their Symbols)
+ * are valid for usage after the OrderBy operator.
+ */
+class OrderBy : public LogicalOperator {
+ public:
+  OrderBy(const std::shared_ptr<LogicalOperator> &input,
+          const std::vector<std::pair<Ordering, Expression *>> order_by,
+          const std::vector<Symbol> remember);
+  void Accept(LogicalOperatorVisitor &visitor) override;
+  std::unique_ptr<Cursor> MakeCursor(GraphDbAccessor &db) override;
+
+ private:
+  // custom Comparator type for comparing lists of TypedValues
+  // does lexicographical ordering of elements based on the above
+  // defined TypedValueCompare, and also accepts a vector of Orderings
+  // the define how respective elements compare
+  class TypedValueListCompare {
+   public:
+    TypedValueListCompare() {}
+    TypedValueListCompare(const std::vector<Ordering> &ordering)
+        : ordering_(ordering) {}
+    bool operator()(const std::list<TypedValue> &c1,
+                    const std::list<TypedValue> &c2) const;
+
+   private:
+    std::vector<Ordering> ordering_;
+  };
+
+  const std::shared_ptr<LogicalOperator> input_;
+  TypedValueListCompare compare_;
+  std::vector<Expression *> order_by_;
+  const std::vector<Symbol> remember_;
+
+  // custom comparison for TypedValue objects
+  // behaves generally like Neo's ORDER BY comparison operator:
+  //  - null is greater than anything else
+  //  - primitives compare naturally, only implicit cast is int->double
+  //  - (list, map, path, vertex, edge) can't compare to anything
+  static bool TypedValueCompare(const TypedValue &a, const TypedValue &b);
+
+  class OrderByCursor : public Cursor {
+   public:
+    OrderByCursor(OrderBy &self, GraphDbAccessor &db);
+    bool Pull(Frame &frame, const SymbolTable &symbol_table) override;
+
+   private:
+    OrderBy &self_;
+    std::unique_ptr<Cursor> input_cursor_;
+    bool did_pull_all_{false};
+    // a cache of elements pulled from the input
+    // first pair element is the order-by list
+    // second pair is the remember list
+    // the cache is filled and sorted (only on first pair elem) on first Pull
+    std::vector<std::pair<std::list<TypedValue>, std::list<TypedValue>>> cache_;
+    // iterator over the cache_, maintains state between Pulls
+    decltype(cache_.begin()) cache_it_ = cache_.begin();
+  };
+};
+
 }  // namespace plan
 }  // namespace query

src/query/typed_value.cpp

@@ -249,6 +249,12 @@ TypedValue::TypedValue(const TypedValue &other) : type_(other.type_) {
   permanent_fail("Unsupported TypedValue::Type");
 }
 
+bool TypedValue::IsNull() const { return type() == TypedValue::Type::Null; }
+
+bool TypedValue::IsNumeric() const {
+  return type() == TypedValue::Type::Int || type() == TypedValue::Type::Double;
+}
+
 std::ostream &operator<<(std::ostream &os, const TypedValue::Type type) {
   switch (type) {
     case TypedValue::Type::Null:
@@ -415,13 +421,12 @@ TypedValue operator<(const TypedValue &a, const TypedValue &b) {
     throw TypedValueException("Invalid 'less' operand types({} + {})", a.type(),
                               b.type());
 
-  if (a.type() == TypedValue::Type::Null || b.type() == TypedValue::Type::Null)
-    return TypedValue::Null;
+  if (a.IsNull() || b.IsNull()) return TypedValue::Null;
 
   if (a.type() == TypedValue::Type::String ||
       b.type() == TypedValue::Type::String) {
     if (a.type() != b.type()) {
-      throw TypedValueException("Invalid equality operand types({} + {})",
+      throw TypedValueException("Invalid 'less' operand types({} + {})",
                                 a.type(), b.type());
     } else {
       return a.Value<std::string>() < b.Value<std::string>();
@@ -445,16 +450,11 @@ TypedValue operator<(const TypedValue &a, const TypedValue &b) {
  * this file at 2017-04-12.
  */
 TypedValue operator==(const TypedValue &a, const TypedValue &b) {
-  if (a.type() == TypedValue::Type::Null || b.type() == TypedValue::Type::Null)
-    return TypedValue::Null;
+  if (a.IsNull() || b.IsNull()) return TypedValue::Null;
 
   // check we have values that can be compared
   // this means that either they're the same type, or (int, double) combo
-  if (!(a.type() == b.type() || (a.type() == TypedValue::Type::Double &&
-                                 b.type() == TypedValue::Type::Int) ||
-        (b.type() == TypedValue::Type::Double &&
-         a.type() == TypedValue::Type::Int)))
-    return false;
+  if ((a.type() != b.type() && !(a.IsNumeric() && b.IsNumeric()))) return false;
 
   switch (a.type()) {
     case TypedValue::Type::Bool:
@@ -500,9 +500,7 @@ TypedValue operator==(const TypedValue &a, const TypedValue &b) {
         auto found_b_it = map_b.find(kv_a.first);
         if (found_b_it == map_b.end()) return false;
         TypedValue comparison = kv_a.second == found_b_it->second;
-        if (comparison.type() == TypedValue::Type::Null ||
-            !comparison.Value<bool>())
-          return false;
+        if (comparison.IsNull() || !comparison.Value<bool>()) return false;
       }
       return true;
     }
@@ -600,8 +598,7 @@ inline void EnsureArithmeticallyOk(const TypedValue &a, const TypedValue &b,
 }
 
 TypedValue operator+(const TypedValue &a, const TypedValue &b) {
-  if (a.type() == TypedValue::Type::Null || b.type() == TypedValue::Type::Null)
-    return TypedValue::Null;
+  if (a.IsNull() || b.IsNull()) return TypedValue::Null;
 
   if (a.type() == TypedValue::Type::List ||
       b.type() == TypedValue::Type::List) {
@@ -638,8 +635,7 @@ TypedValue operator+(const TypedValue &a, const TypedValue &b) {
 TypedValue operator-(const TypedValue &a, const TypedValue &b) {
   EnsureArithmeticallyOk(a, b, false, "subtraction");
 
-  if (a.type() == TypedValue::Type::Null || b.type() == TypedValue::Type::Null)
-    return TypedValue::Null;
+  if (a.IsNull() || b.IsNull()) return TypedValue::Null;
 
   // at this point we only have int and double
   if (a.type() == TypedValue::Type::Double ||
@@ -653,8 +649,7 @@ TypedValue operator-(const TypedValue &a, const TypedValue &b) {
 TypedValue operator/(const TypedValue &a, const TypedValue &b) {
   EnsureArithmeticallyOk(a, b, false, "division");
 
-  if (a.type() == TypedValue::Type::Null || b.type() == TypedValue::Type::Null)
-    return TypedValue::Null;
+  if (a.IsNull() || b.IsNull()) return TypedValue::Null;
 
   // at this point we only have int and double
   if (a.type() == TypedValue::Type::Double ||
@@ -670,8 +665,7 @@ TypedValue operator/(const TypedValue &a, const TypedValue &b) {
 TypedValue operator*(const TypedValue &a, const TypedValue &b) {
   EnsureArithmeticallyOk(a, b, false, "multiplication");
 
-  if (a.type() == TypedValue::Type::Null || b.type() == TypedValue::Type::Null)
-    return TypedValue::Null;
+  if (a.IsNull() || b.IsNull()) return TypedValue::Null;
 
   // at this point we only have int and double
   if (a.type() == TypedValue::Type::Double ||
@@ -685,8 +679,7 @@ TypedValue operator*(const TypedValue &a, const TypedValue &b) {
 TypedValue operator%(const TypedValue &a, const TypedValue &b) {
   EnsureArithmeticallyOk(a, b, false, "modulo");
 
-  if (a.type() == TypedValue::Type::Null || b.type() == TypedValue::Type::Null)
-    return TypedValue::Null;
+  if (a.IsNull() || b.IsNull()) return TypedValue::Null;
 
   // at this point we only have int and double
   if (a.type() == TypedValue::Type::Double ||
@@ -714,7 +707,7 @@ TypedValue operator&&(const TypedValue &a, const TypedValue &b) {
   // if either operand is false, the result is false
   if (a.type() == TypedValue::Type::Bool && !a.Value<bool>()) return false;
   if (b.type() == TypedValue::Type::Bool && !b.Value<bool>()) return false;
-  if (a.type() == TypedValue::Type::Null || b.type() == TypedValue::Type::Null)
+  if (a.IsNull() || b.IsNull())
     return TypedValue::Null;
   // neither is false, neither is null, thus both are true
   return true;
@@ -726,26 +719,26 @@ TypedValue operator||(const TypedValue &a, const TypedValue &b) {
   // if either operand is true, the result is true
   if (a.type() == TypedValue::Type::Bool && a.Value<bool>()) return true;
   if (b.type() == TypedValue::Type::Bool && b.Value<bool>()) return true;
-  if (a.type() == TypedValue::Type::Null || b.type() == TypedValue::Type::Null)
+  if (a.IsNull() || b.IsNull())
     return TypedValue::Null;
   // neither is true, neither is null, thus both are false
   return false;
 }
 
 TypedValue operator^(const TypedValue &a, const TypedValue &b) {
-  EnsureLogicallyOk(a, b, "logical XOR");
+  EnsureLogicallyOk(a,b, "logical XOR");
   // at this point we only have null and bool
-  if (a.type() == TypedValue::Type::Null || b.type() == TypedValue::Type::Null)
-    return TypedValue::Null;
-  else
-    return static_cast<bool>(a.Value<bool>() ^ b.Value<bool>());
+    if (a.IsNull() ||
+        b.IsNull())
+      return TypedValue::Null;
+     else
+      return static_cast<bool>(a.Value<bool>() ^ b.Value<bool>());
+
 }
 
 bool TypedValue::BoolEqual::operator()(const TypedValue &lhs,
                                        const TypedValue &rhs) const {
-  if (lhs.type() == TypedValue::Type::Null &&
-      rhs.type() == TypedValue::Type::Null)
-    return true;
+  if (lhs.IsNull() && rhs.IsNull()) return true;
   TypedValue equality_result = lhs == rhs;
   switch (equality_result.type()) {
     case TypedValue::Type::Bool:

src/query/typed_value.hpp

@@ -130,6 +130,13 @@ class TypedValue : public TotalOrdering<TypedValue, TypedValue, TypedValue> {
   template <typename T>
   const T &Value() const;
 
+  /** Convenience function for checking if this TypedValue is Null */
+  bool IsNull() const;
+
+  /** Convenience function for checking if this TypedValue is either
+   * an integer or double */
+  bool IsNumeric() const;
+
   friend std::ostream &operator<<(std::ostream &stream, const TypedValue &prop);
 
  private:

tests/unit/query_plan_bag_semantics.cpp

@@ -3,6 +3,7 @@
 // Created by Florijan Stamenkovic on 14.03.17.
 //
 
+#include <algorithm>
 #include <iterator>
 #include <memory>
 #include <vector>
@@ -45,8 +46,7 @@ TEST(QueryPlan, Skip) {
   dba->advance_command();
   EXPECT_EQ(1, PullAll(skip, *dba, symbol_table));
 
-  for (int i = 0; i < 10; ++i)
-    dba->insert_vertex();
+  for (int i = 0; i < 10; ++i) dba->insert_vertex();
   dba->advance_command();
   EXPECT_EQ(11, PullAll(skip, *dba, symbol_table));
 }
@@ -75,8 +75,7 @@ TEST(QueryPlan, Limit) {
   dba->advance_command();
   EXPECT_EQ(2, PullAll(skip, *dba, symbol_table));
 
-  for (int i = 0; i < 10; ++i)
-    dba->insert_vertex();
+  for (int i = 0; i < 10; ++i) dba->insert_vertex();
   dba->advance_command();
   EXPECT_EQ(2, PullAll(skip, *dba, symbol_table));
 }
@@ -104,3 +103,166 @@ TEST(QueryPlan, CreateLimit) {
   dba->advance_command();
   EXPECT_EQ(3, CountIterable(dba->vertices()));
 }
+
+TEST(QueryPlan, OrderBy) {
+  Dbms dbms;
+  auto dba = dbms.active();
+  AstTreeStorage storage;
+  SymbolTable symbol_table;
+  auto prop = dba->property("prop");
+
+  // contains a series of tests
+  // each test defines the ordering a vector of values in the desired order
+  auto Null = TypedValue::Null;
+  std::vector<std::pair<Ordering, std::vector<TypedValue>>> orderable{
+      {Ordering::ASC, {0, 0, 0.5, 1, 2, 12.6, 42, Null, Null}},
+      {Ordering::ASC, {false, false, true, true, Null, Null}},
+      {Ordering::ASC, {"A", "B", "a", "a", "aa", "ab", "aba", Null, Null}},
+      {Ordering::DESC, {Null, Null, 33, 33, 32.5, 32, 2.2, 2.1, 0}},
+      {Ordering::DESC, {Null, true, false}},
+      {Ordering::DESC, {Null, "zorro", "borro"}}};
+
+  for (const auto &order_value_pair : orderable) {
+    const auto &values = order_value_pair.second;
+    // empty database
+    for (auto &vertex : dba->vertices()) dba->detach_remove_vertex(vertex);
+    dba->advance_command();
+    ASSERT_EQ(0, CountIterable(dba->vertices()));
+
+    // take some effort to shuffle the values
+    // because we are testing that something not ordered gets ordered
+    // and need to take care it does not happen by accident
+    std::vector<TypedValue> shuffled(values.begin(), values.end());
+    auto order_equal = [&values, &shuffled]() {
+      return std::equal(values.begin(), values.end(), shuffled.begin(),
+                        TypedValue::BoolEqual{});
+    };
+    for (int i = 0; i < 50 && order_equal(); ++i) {
+      std::random_shuffle(shuffled.begin(), shuffled.end());
+    }
+    ASSERT_FALSE(order_equal());
+
+    // create the vertices
+    for (const auto &value : shuffled)
+      dba->insert_vertex().PropsSet(prop, value);
+    dba->advance_command();
+
+    // order by and collect results
+    auto n = MakeScanAll(storage, symbol_table, "n");
+    auto n_p = PROPERTY_LOOKUP("n", prop);
+    symbol_table[*n_p->expression_] = n.sym_;
+    auto order_by = std::make_shared<plan::OrderBy>(
+        n.op_,
+        std::vector<std::pair<Ordering, Expression *>>{
+            {order_value_pair.first, n_p}},
+        std::vector<Symbol>{n.sym_});
+    auto n_p_ne = NEXPR("n.p", n_p);
+    symbol_table[*n_p_ne] = symbol_table.CreateSymbol("n.p");
+    auto produce = MakeProduce(order_by, n_p_ne);
+    auto results = CollectProduce(produce, symbol_table, *dba).GetResults();
+    ASSERT_EQ(values.size(), results.size());
+    for (int j = 0; j < results.size(); ++j)
+      EXPECT_TRUE(TypedValue::BoolEqual{}(results[j][0], values[j]));
+  }
+}
+
+TEST(QueryPlan, OrderByMultiple) {
+  Dbms dbms;
+  auto dba = dbms.active();
+  AstTreeStorage storage;
+  SymbolTable symbol_table;
+
+  auto p1 = dba->property("p1");
+  auto p2 = dba->property("p2");
+
+  // create a bunch of vertices that in two properties
+  // have all the variations (with repetition) of N values.
+  // ensure that those vertices are not created in the
+  // "right" sequence, but randomized
+  const int N = 20;
+  std::vector<std::pair<int, int>> prop_values;
+  for (int i = 0; i < N * N; ++i) prop_values.emplace_back(i % N, i / N);
+  std::random_shuffle(prop_values.begin(), prop_values.end());
+  for (const auto &pair : prop_values) {
+    auto v = dba->insert_vertex();
+    v.PropsSet(p1, pair.first);
+    v.PropsSet(p2, pair.second);
+  }
+  dba->advance_command();
+
+  // order by and collect results
+  auto n = MakeScanAll(storage, symbol_table, "n");
+  auto n_p1 = PROPERTY_LOOKUP("n", p1);
+  symbol_table[*n_p1->expression_] = n.sym_;
+  auto n_p2 = PROPERTY_LOOKUP("n", p2);
+  symbol_table[*n_p2->expression_] = n.sym_;
+  // order the results so we get
+  // (p1: 0, p2: N-1)
+  // (p1: 0, p2: N-2)
+  // ...
+  // (p1: N-1, p2:0)
+  auto order_by = std::make_shared<plan::OrderBy>(
+      n.op_,
+      std::vector<std::pair<Ordering, Expression *>>{
+          {Ordering::ASC, n_p1}, {Ordering::DESC, n_p2},
+      },
+      std::vector<Symbol>{n.sym_});
+  auto n_p1_ne = NEXPR("n.p1", n_p1);
+  symbol_table[*n_p1_ne] = symbol_table.CreateSymbol("n.p1");
+  auto n_p2_ne = NEXPR("n.p2", n_p2);
+  symbol_table[*n_p2_ne] = symbol_table.CreateSymbol("n.p2");
+  auto produce = MakeProduce(order_by, n_p1_ne, n_p2_ne);
+  auto results = CollectProduce(produce, symbol_table, *dba).GetResults();
+  ASSERT_EQ(N * N, results.size());
+  for (int j = 0; j < N * N; ++j) {
+    ASSERT_EQ(results[j][0].type(), TypedValue::Type::Int);
+    EXPECT_EQ(results[j][0].Value<int64_t>(), j / N);
+    ASSERT_EQ(results[j][1].type(), TypedValue::Type::Int);
+    EXPECT_EQ(results[j][1].Value<int64_t>(), N - 1 - j % N);
+  }
+}
+
+TEST(QueryPlan, OrderByExceptions) {
+  Dbms dbms;
+  auto dba = dbms.active();
+  AstTreeStorage storage;
+  SymbolTable symbol_table;
+  auto prop = dba->property("prop");
+
+  // a vector of pairs of typed values that should result
+  // in an exception when trying to order on them
+  std::vector<std::pair<TypedValue, TypedValue>> exception_pairs{
+      {42, true},
+      {42, "bla"},
+      {42, std::vector<TypedValue>{42}},
+      {true, "bla"},
+      {true, std::vector<TypedValue>{true}},
+      {"bla", std::vector<TypedValue>{"bla"}},
+      // illegal comparisons of same-type values
+      {std::vector<TypedValue>{42}, std::vector<TypedValue>{42}}};
+
+  for (const auto &pair : exception_pairs) {
+    // empty database
+    for (auto &vertex : dba->vertices()) dba->detach_remove_vertex(vertex);
+    dba->advance_command();
+    ASSERT_EQ(0, CountIterable(dba->vertices()));
+
+    // make two vertices, and set values
+    dba->insert_vertex().PropsSet(prop, pair.first);
+    dba->insert_vertex().PropsSet(prop, pair.second);
+    dba->advance_command();
+    ASSERT_EQ(2, CountIterable(dba->vertices()));
+    for (const auto &va : dba->vertices())
+      ASSERT_NE(va.PropsAt(prop).type(), PropertyValue::Type::Null);
+
+    // order by and expect an exception
+    auto n = MakeScanAll(storage, symbol_table, "n");
+    auto n_p = PROPERTY_LOOKUP("n", prop);
+    symbol_table[*n_p->expression_] = n.sym_;
+    auto order_by = std::make_shared<plan::OrderBy>(
+        n.op_,
+        std::vector<std::pair<Ordering, Expression *>>{{Ordering::ASC, n_p}},
+        std::vector<Symbol>{});
+    EXPECT_THROW(PullAll(order_by, *dba, symbol_table), QueryRuntimeException);
+  }
+}

tests/unit/typed_value.cpp

@@ -36,9 +36,7 @@ void EXPECT_PROP_EQ(const TypedValue &a, const TypedValue &b) {
   EXPECT_PROP_TRUE(a == b);
 }
 
-void EXPECT_PROP_ISNULL(const TypedValue &a) {
-  EXPECT_TRUE(a.type() == TypedValue::Type::Null);
-}
+void EXPECT_PROP_ISNULL(const TypedValue &a) { EXPECT_TRUE(a.IsNull()); }
 
 void EXPECT_PROP_NE(const TypedValue &a, const TypedValue &b) {
   EXPECT_PROP_TRUE(a != b);
@@ -109,8 +107,9 @@ TEST(TypedValue, Equals) {
                  TypedValue(std::map<std::string, TypedValue>{{"b", 1}}));
   EXPECT_PROP_NE(TypedValue(std::map<std::string, TypedValue>{{"a", 1}}),
                  TypedValue(std::map<std::string, TypedValue>{{"a", 2}}));
-  EXPECT_PROP_NE(TypedValue(std::map<std::string, TypedValue>{{"a", 1}}),
-                 TypedValue(std::map<std::string, TypedValue>{{"a", 1}, {"b", 1}}));
+  EXPECT_PROP_NE(
+      TypedValue(std::map<std::string, TypedValue>{{"a", 1}}),
+      TypedValue(std::map<std::string, TypedValue>{{"a", 1}, {"b", 1}}));
 }
 
 TEST(TypedValue, BoolEquals) {
@@ -365,10 +364,8 @@ void TestLogicalThrows(
     for (int j = 0; j < (int)props.size(); ++j) {
       auto p2 = props.at(j);
       // skip situations when both p1 and p2 are either bool or null
-      auto p1_ok = p1.type() == TypedValue::Type::Bool ||
-                   p1.type() == TypedValue::Type::Null;
-      auto p2_ok = p2.type() == TypedValue::Type::Bool ||
-                   p2.type() == TypedValue::Type::Null;
+      auto p1_ok = p1.type() == TypedValue::Type::Bool || p1.IsNull();
+      auto p2_ok = p2.type() == TypedValue::Type::Bool || p2.IsNull();
       if (p1_ok && p2_ok) continue;
 
       EXPECT_THROW(op(p1, p2), TypedValueException);