diff --git a/src/query/context.hpp b/src/query/context.hpp index 8dfe09ecc..3040d6e10 100644 --- a/src/query/context.hpp +++ b/src/query/context.hpp @@ -51,8 +51,6 @@ struct EvaluationContext { /// All counters generated by `counter` function, mutable because the function /// modifies the values mutable std::unordered_map counters{}; - /// Property lookup cache ({symbol: {property_id: property_value, ...}, ...}) - mutable std::unordered_map> property_lookups_cache{}; }; inline std::vector NamesToProperties(const std::vector &property_names, diff --git a/src/query/frontend/semantic/symbol_generator.cpp b/src/query/frontend/semantic/symbol_generator.cpp index 5fbc5d8a3..038bdf66b 100644 --- a/src/query/frontend/semantic/symbol_generator.cpp +++ b/src/query/frontend/semantic/symbol_generator.cpp @@ -84,6 +84,9 @@ void SymbolGenerator::VisitReturnBody(ReturnBody &body, Where *where) { for (auto &expr : body.named_expressions) { expr->Accept(*this); } + + SetEvaluationModeOnPropertyLookups(body); + std::vector user_symbols; if (body.all_identifiers) { // Carry over user symbols because '*' appeared. @@ -400,26 +403,8 @@ SymbolGenerator::ReturnType SymbolGenerator::Visit(Identifier &ident) { return true; } -bool SymbolGenerator::PostVisit(MapLiteral &map_literal) { - std::unordered_map property_lookups{}; - - for (const auto &pair : map_literal.elements_) { - if (pair.second->GetTypeInfo() != PropertyLookup::kType) continue; - auto *property_lookup = static_cast(pair.second); - if (property_lookup->expression_->GetTypeInfo() != Identifier::kType) continue; - - auto symbol_pos = static_cast(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); - } - } - +bool SymbolGenerator::PreVisit(MapLiteral &map_literal) { + SetEvaluationModeOnPropertyLookups(map_literal); return true; } @@ -741,4 +726,31 @@ bool SymbolGenerator::ConsumePredefinedIdentifier(const std::string &name) { return true; } +void PropertyLookupEvaluationModeVisitor::Visit(PropertyLookup &property_lookup) { + if (property_lookup.expression_->GetTypeInfo() != Identifier::kType) { + return; + } + + auto identifier_symbol = static_cast(property_lookup.expression_)->name_; + + if (this->gather_property_lookup_counts) { + if (!property_lookup_counts_by_symbol.contains(identifier_symbol)) { + property_lookup_counts_by_symbol[identifier_symbol] = 0; + } + + property_lookup_counts_by_symbol[identifier_symbol]++; + + return; + } + + if (this->assign_property_lookup_evaluations) { + if (property_lookup_counts_by_symbol.contains(identifier_symbol) && + property_lookup_counts_by_symbol[identifier_symbol] > 1) { + property_lookup.evaluation_mode_ = PropertyLookup::EvaluationMode::GET_ALL_PROPERTIES; + } + + return; + } +} + } // namespace memgraph::query diff --git a/src/query/frontend/semantic/symbol_generator.hpp b/src/query/frontend/semantic/symbol_generator.hpp index b2f7a928c..9e4fc32b5 100644 --- a/src/query/frontend/semantic/symbol_generator.hpp +++ b/src/query/frontend/semantic/symbol_generator.hpp @@ -72,8 +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; + bool PreVisit(MapLiteral &) override; + bool PostVisit(MapLiteral &) override { return true; }; ReturnType Visit(ParameterLookup &) override { return true; } bool PreVisit(Aggregation &) override; bool PostVisit(Aggregation &) override; @@ -177,6 +177,86 @@ class SymbolGenerator : public HierarchicalTreeVisitor { std::vector scopes_; }; +/// Visits the AST and assigns the evaluation mode for all the property lookups +/// If property lookup for one symbol is visited more times, it is better to fetch all properties +class PropertyLookupEvaluationModeVisitor : public ExpressionVisitor { + public: + explicit PropertyLookupEvaluationModeVisitor() {} + + using ExpressionVisitor::Visit; + + // Unary operators + void Visit(NotOperator &op) override { op.expression_->Accept(*this); } + void Visit(IsNullOperator &op) override { op.expression_->Accept(*this); }; + void Visit(UnaryPlusOperator &op) override{}; + void Visit(UnaryMinusOperator &op) override{}; + + void Visit(OrOperator &op) override { + op.expression1_->Accept(*this); + op.expression2_->Accept(*this); + } + void Visit(XorOperator &op) override { + op.expression1_->Accept(*this); + op.expression2_->Accept(*this); + } + void Visit(AndOperator &op) override { + op.expression1_->Accept(*this); + op.expression2_->Accept(*this); + } + void Visit(NotEqualOperator &op) override { + op.expression1_->Accept(*this); + op.expression2_->Accept(*this); + }; + void Visit(EqualOperator &op) override { + op.expression1_->Accept(*this); + op.expression2_->Accept(*this); + }; + void Visit(InListOperator &op) override { + op.expression1_->Accept(*this); + op.expression2_->Accept(*this); + }; + void Visit(AdditionOperator &op) override{}; + void Visit(SubtractionOperator &op) override{}; + void Visit(MultiplicationOperator &op) override{}; + void Visit(DivisionOperator &op) override{}; + void Visit(ModOperator &op) override{}; + void Visit(LessOperator &op) override{}; + void Visit(GreaterOperator &op) override{}; + void Visit(LessEqualOperator &op) override{}; + void Visit(GreaterEqualOperator &op) override{}; + void Visit(SubscriptOperator &op) override{}; + void Visit(ListSlicingOperator &op) override{}; + void Visit(IfOperator &op) override{}; + void Visit(ListLiteral &op) override{}; + void Visit(MapLiteral &op) override{}; + void Visit(MapProjectionLiteral &op) override{}; + void Visit(LabelsTest &op) override{}; + void Visit(Aggregation &op) override{}; + void Visit(Function &op) override{}; + void Visit(Reduce &op) override{}; + void Visit(Coalesce &op) override{}; + void Visit(Extract &op) override{}; + void Visit(Exists &op) override{}; + void Visit(All &op) override{}; + void Visit(Single &op) override{}; + void Visit(Any &op) override{}; + void Visit(None &op) override{}; + void Visit(Identifier &op) override{}; + void Visit(PrimitiveLiteral &op) override{}; + void Visit(AllPropertiesLookup &op) override{}; + void Visit(ParameterLookup &op) override{}; + void Visit(NamedExpression &op) override { op.expression_->Accept(*this); }; + void Visit(RegexMatch &op) override{}; + + void Visit(PropertyLookup & /*property_lookup*/) override; + + bool gather_property_lookup_counts{false}; + bool assign_property_lookup_evaluations{false}; + + private: + std::unordered_map property_lookup_counts_by_symbol{}; +}; + inline SymbolTable MakeSymbolTable(CypherQuery *query, const std::vector &predefined_identifiers = {}) { SymbolTable symbol_table; SymbolGenerator symbol_generator(&symbol_table, predefined_identifiers); @@ -187,4 +267,36 @@ inline SymbolTable MakeSymbolTable(CypherQuery *query, const std::vectorAccept(visitor); + } + visitor.gather_property_lookup_counts = false; + + visitor.assign_property_lookup_evaluations = true; + for (auto *expr : body.named_expressions) { + expr->Accept(visitor); + } + visitor.assign_property_lookup_evaluations = false; +} + +inline void SetEvaluationModeOnPropertyLookups(MapLiteral &map_literal) { + PropertyLookupEvaluationModeVisitor visitor; + + visitor.gather_property_lookup_counts = true; + for (auto &pair : map_literal.elements_) { + pair.second->Accept(visitor); + } + visitor.gather_property_lookup_counts = false; + + visitor.assign_property_lookup_evaluations = true; + for (auto &pair : map_literal.elements_) { + pair.second->Accept(visitor); + } + visitor.assign_property_lookup_evaluations = false; +} + } // namespace memgraph::query diff --git a/src/query/interpret/eval.hpp b/src/query/interpret/eval.hpp index 47a7cb598..d4c2c8bc0 100644 --- a/src/query/interpret/eval.hpp +++ b/src/query/interpret/eval.hpp @@ -548,13 +548,13 @@ class ExpressionEvaluator : public ExpressionVisitor { case TypedValue::Type::Vertex: if (property_lookup.evaluation_mode_ == PropertyLookup::EvaluationMode::GET_ALL_PROPERTIES) { auto symbol_pos = static_cast(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())); + if (!property_lookup_cache_.contains(symbol_pos)) { + property_lookup_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); + if (property_lookup_cache_[symbol_pos].contains(property_id)) { + return TypedValue(property_lookup_cache_[symbol_pos][property_id], ctx_->memory); } return TypedValue(ctx_->memory); } else { @@ -563,13 +563,13 @@ class ExpressionEvaluator : public ExpressionVisitor { case TypedValue::Type::Edge: if (property_lookup.evaluation_mode_ == PropertyLookup::EvaluationMode::GET_ALL_PROPERTIES) { auto symbol_pos = static_cast(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())); + if (!property_lookup_cache_.contains(symbol_pos)) { + property_lookup_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); + if (property_lookup_cache_[symbol_pos].contains(property_id)) { + return TypedValue(property_lookup_cache_[symbol_pos][property_id], ctx_->memory); } return TypedValue(ctx_->memory); } else { @@ -784,10 +784,6 @@ class ExpressionEvaluator : public ExpressionVisitor { 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); } @@ -1171,6 +1167,8 @@ class ExpressionEvaluator : public ExpressionVisitor { // which switching approach should be used when evaluating storage::View view_; FrameChangeCollector *frame_change_collector_; + /// Property lookup cache ({symbol: {property_id: property_value, ...}, ...}) + mutable std::unordered_map> property_lookup_cache_{}; }; // namespace memgraph::query /// A helper function for evaluating an expression that's an int. diff --git a/tests/gql_behave/tests/memgraph_V1/features/with.feature b/tests/gql_behave/tests/memgraph_V1/features/with.feature index 46c48a1d1..f1882e8d7 100644 --- a/tests/gql_behave/tests/memgraph_V1/features/with.feature +++ b/tests/gql_behave/tests/memgraph_V1/features/with.feature @@ -250,3 +250,17 @@ Feature: With | n | | ({id: 0}) | + Scenario: With test 16: + Given an empty graph + And having executed: + """ + CREATE ({id: 0}) CREATE ({id:1}); + """ + When executing query: + """ + MATCH (n) RETURN n.id AS id; + """ + Then the result should be: + | id | + | 0 | + | 1 | diff --git a/tests/gql_behave/tests/memgraph_V1_on_disk/features/with.feature b/tests/gql_behave/tests/memgraph_V1_on_disk/features/with.feature index 1541df75e..f1882e8d7 100644 --- a/tests/gql_behave/tests/memgraph_V1_on_disk/features/with.feature +++ b/tests/gql_behave/tests/memgraph_V1_on_disk/features/with.feature @@ -249,3 +249,18 @@ Feature: With Then the result should be: | n | | ({id: 0}) | + + Scenario: With test 16: + Given an empty graph + And having executed: + """ + CREATE ({id: 0}) CREATE ({id:1}); + """ + When executing query: + """ + MATCH (n) RETURN n.id AS id; + """ + Then the result should be: + | id | + | 0 | + | 1 | diff --git a/tests/unit/query_plan_create_set_remove_delete.cpp b/tests/unit/query_plan_create_set_remove_delete.cpp index b50bd04a8..d0127c7c6 100644 --- a/tests/unit/query_plan_create_set_remove_delete.cpp +++ b/tests/unit/query_plan_create_set_remove_delete.cpp @@ -1763,7 +1763,6 @@ TYPED_TEST(QueryPlanTest, SetPropertiesFromMapWithCaching) { expected_properties.emplace(dba.NameToProperty("prop2"), memgraph::storage::PropertyValue(44)); expected_properties.emplace(dba.NameToProperty("new_prop1"), memgraph::storage::PropertyValue(43)); expected_properties.emplace(dba.NameToProperty("new_prop2"), memgraph::storage::PropertyValue(44)); - EXPECT_EQ(context.evaluation_context.property_lookups_cache.size(), 0); EXPECT_EQ(new_properties.HasError(), false); EXPECT_EQ(*new_properties, expected_properties); }