Property lookup caching (#1168)

src/query/context.hpp

@@ -51,6 +51,8 @@ struct EvaluationContext {
   /// All counters generated by `counter` function, mutable because the function
   /// modifies the values
   mutable std::unordered_map<std::string, int64_t> counters{};
+  /// Property lookup cache ({symbol: {property_id: property_value, ...}, ...})
+  mutable std::unordered_map<int32_t, std::map<storage::PropertyId, storage::PropertyValue>> property_lookups_cache{};
 };
 
 inline std::vector<storage::PropertyId> NamesToProperties(const std::vector<std::string> &property_names,

src/query/frontend/ast/ast.hpp

@@ -1186,6 +1186,8 @@ class PropertyLookup : public memgraph::query::Expression {
   static const utils::TypeInfo kType;
   const utils::TypeInfo &GetTypeInfo() const override { return kType; }
 
+  enum class EvaluationMode { GET_OWN_PROPERTY, GET_ALL_PROPERTIES };
+
   PropertyLookup() = default;
 
   DEFVISITABLE(ExpressionVisitor<TypedValue>);
@@ -1200,11 +1202,13 @@ class PropertyLookup : public memgraph::query::Expression {
 
   memgraph::query::Expression *expression_{nullptr};
   memgraph::query::PropertyIx property_;
+  memgraph::query::PropertyLookup::EvaluationMode evaluation_mode_{EvaluationMode::GET_OWN_PROPERTY};
 
   PropertyLookup *Clone(AstStorage *storage) const override {
     PropertyLookup *object = storage->Create<PropertyLookup>();
     object->expression_ = expression_ ? expression_->Clone(storage) : nullptr;
     object->property_ = storage->GetPropertyIx(property_.name);
+    object->evaluation_mode_ = evaluation_mode_;
     return object;
   }
 

src/query/frontend/semantic/symbol_generator.cpp

@@ -400,6 +400,29 @@ SymbolGenerator::ReturnType SymbolGenerator::Visit(Identifier &ident) {
   return true;
 }
 
+bool SymbolGenerator::PostVisit(MapLiteral &map_literal) {
+  std::unordered_map<int32_t, PropertyLookup *> property_lookups{};
+
+  for (const auto &pair : map_literal.elements_) {
+    if (pair.second->GetTypeInfo() != PropertyLookup::kType) continue;
+    auto *property_lookup = static_cast<PropertyLookup *>(pair.second);
+    if (property_lookup->expression_->GetTypeInfo() != Identifier::kType) continue;
+
+    auto symbol_pos = static_cast<Identifier *>(property_lookup->expression_)->symbol_pos_;
+    try {
+      auto *existing_property_lookup = property_lookups.at(symbol_pos);
+      // If already there (no exception), update the original and current PropertyLookups
+      existing_property_lookup->evaluation_mode_ = PropertyLookup::EvaluationMode::GET_ALL_PROPERTIES;
+      property_lookup->evaluation_mode_ = PropertyLookup::EvaluationMode::GET_ALL_PROPERTIES;
+    } catch (const std::out_of_range &) {
+      // Otherwise, add the PropertyLookup to the map
+      property_lookups.emplace(symbol_pos, property_lookup);
+    }
+  }
+
+  return true;
+}
+
 bool SymbolGenerator::PreVisit(Aggregation &aggr) {
   auto &scope = scopes_.back();
   // Check if the aggregation can be used in this context. This check should

src/query/frontend/semantic/symbol_generator.hpp

@@ -72,6 +72,8 @@ class SymbolGenerator : public HierarchicalTreeVisitor {
   // Expressions
   ReturnType Visit(Identifier &) override;
   ReturnType Visit(PrimitiveLiteral &) override { return true; }
+  bool PreVisit(MapLiteral &) override { return true; }
+  bool PostVisit(MapLiteral &) override;
   ReturnType Visit(ParameterLookup &) override { return true; }
   bool PreVisit(Aggregation &) override;
   bool PostVisit(Aggregation &) override;

src/query/interpret/eval.hpp

@@ -546,9 +546,35 @@ class ExpressionEvaluator : public ExpressionVisitor<TypedValue> {
       case TypedValue::Type::Null:
         return TypedValue(ctx_->memory);
       case TypedValue::Type::Vertex:
-        return TypedValue(GetProperty(expression_result_ptr->ValueVertex(), property_lookup.property_), ctx_->memory);
+        if (property_lookup.evaluation_mode_ == PropertyLookup::EvaluationMode::GET_ALL_PROPERTIES) {
+          auto symbol_pos = static_cast<Identifier *>(property_lookup.expression_)->symbol_pos_;
+          if (!ctx_->property_lookups_cache.contains(symbol_pos)) {
+            ctx_->property_lookups_cache.emplace(symbol_pos, GetAllProperties(expression_result_ptr->ValueVertex()));
+          }
+
+          auto property_id = ctx_->properties[property_lookup.property_.ix];
+          if (ctx_->property_lookups_cache[symbol_pos].contains(property_id)) {
+            return TypedValue(ctx_->property_lookups_cache[symbol_pos][property_id], ctx_->memory);
+          }
+          return TypedValue(ctx_->memory);
+        } else {
+          return TypedValue(GetProperty(expression_result_ptr->ValueVertex(), property_lookup.property_), ctx_->memory);
+        }
       case TypedValue::Type::Edge:
-        return TypedValue(GetProperty(expression_result_ptr->ValueEdge(), property_lookup.property_), ctx_->memory);
+        if (property_lookup.evaluation_mode_ == PropertyLookup::EvaluationMode::GET_ALL_PROPERTIES) {
+          auto symbol_pos = static_cast<Identifier *>(property_lookup.expression_)->symbol_pos_;
+          if (!ctx_->property_lookups_cache.contains(symbol_pos)) {
+            ctx_->property_lookups_cache.emplace(symbol_pos, GetAllProperties(expression_result_ptr->ValueEdge()));
+          }
+
+          auto property_id = ctx_->properties[property_lookup.property_.ix];
+          if (ctx_->property_lookups_cache[symbol_pos].contains(property_id)) {
+            return TypedValue(ctx_->property_lookups_cache[symbol_pos][property_id], ctx_->memory);
+          }
+          return TypedValue(ctx_->memory);
+        } else {
+          return TypedValue(GetProperty(expression_result_ptr->ValueEdge(), property_lookup.property_), ctx_->memory);
+        }
       case TypedValue::Type::Map: {
         auto &map = expression_result_ptr->ValueMap();
         auto found = map.find(property_lookup.property_.name.c_str());
@@ -754,7 +780,14 @@ class ExpressionEvaluator : public ExpressionVisitor<TypedValue> {
 
   TypedValue Visit(MapLiteral &literal) override {
     TypedValue::TMap result(ctx_->memory);
-    for (const auto &pair : literal.elements_) result.emplace(pair.first.name, pair.second->Accept(*this));
+    for (const auto &pair : literal.elements_) {
+      result.emplace(pair.first.name, pair.second->Accept(*this));
+    }
+
+    ctx_->property_lookups_cache.clear();
+    // TODO Don’t clear the cache if there are remaining MapLiterals with PropertyLookups that read the same properties
+    // from the same variable (symbol & value)
+
     return TypedValue(result, ctx_->memory);
   }
 
@@ -1048,6 +1081,33 @@ class ExpressionEvaluator : public ExpressionVisitor<TypedValue> {
   }
 
  private:
+  template <class TRecordAccessor>
+  std::map<storage::PropertyId, storage::PropertyValue> GetAllProperties(const TRecordAccessor &record_accessor) {
+    auto maybe_props = record_accessor.Properties(view_);
+    if (maybe_props.HasError() && maybe_props.GetError() == storage::Error::NONEXISTENT_OBJECT) {
+      // This is a very nasty and temporary hack in order to make MERGE work.
+      // The old storage had the following logic when returning an `OLD` view:
+      // `return old ? old : new`. That means that if the `OLD` view didn't
+      // exist, it returned the NEW view. With this hack we simulate that
+      // behavior.
+      // TODO (mferencevic, teon.banek): Remove once MERGE is reimplemented.
+      maybe_props = record_accessor.Properties(storage::View::NEW);
+    }
+    if (maybe_props.HasError()) {
+      switch (maybe_props.GetError()) {
+        case storage::Error::DELETED_OBJECT:
+          throw QueryRuntimeException("Trying to get properties from a deleted object.");
+        case storage::Error::NONEXISTENT_OBJECT:
+          throw query::QueryRuntimeException("Trying to get properties from an object that doesn't exist.");
+        case storage::Error::SERIALIZATION_ERROR:
+        case storage::Error::VERTEX_HAS_EDGES:
+        case storage::Error::PROPERTIES_DISABLED:
+          throw QueryRuntimeException("Unexpected error when getting properties.");
+      }
+    }
+    return *maybe_props;
+  }
+
   template <class TRecordAccessor>
   storage::PropertyValue GetProperty(const TRecordAccessor &record_accessor, PropertyIx prop) {
     auto maybe_prop = record_accessor.GetProperty(view_, ctx_->properties[prop.ix]);