tursom/memgraph
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Fix nested FOREACH shadowing bug ()

Browse Source
This commit is contained in:
Bruno Sačarić 2023-01-25 20:06:05 +01:00 committed by GitHub
parent d3f275b231
commit 34dd47ef07
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 28 additions and 58 deletions
src/query/frontend/semantic
symbol_generator.cppsymbol_generator.hpp
tests
gql_behave/tests/memgraph_V1/features
foreach.feature
unit
query_semantic.cpp

38
src/query/frontend/semantic/symbol_generator.cpp
View File

@ -1,4 +1,4 @@
// Copyright 2022 Memgraph Ltd.
// Copyright 2023 Memgraph Ltd.
//
// Use of this software is governed by the Business Source License
// included in the file licenses/BSL.txt; by using this file, you agree to be bound by the terms of the Business Source
@ -64,14 +64,6 @@ auto SymbolGenerator::CreateSymbol(const std::string &name, bool user_declared,
return symbol;
}
auto SymbolGenerator::GetOrCreateSymbolLocalScope(const std::string &name, bool user_declared, Symbol::Type type) {
auto &scope = scopes_.back();
if (auto maybe_symbol = FindSymbolInScope(name, scope, type); maybe_symbol) {
return *maybe_symbol;
}
return CreateSymbol(name, user_declared, type);
}
auto SymbolGenerator::GetOrCreateSymbol(const std::string &name, bool user_declared, Symbol::Type type) {
// NOLINTNEXTLINE
for (auto scope = scopes_.rbegin(); scope != scopes_.rend(); ++scope) {
@ -206,7 +198,7 @@ bool SymbolGenerator::PreVisit(CallProcedure &call_proc) {
bool SymbolGenerator::PostVisit(CallProcedure &call_proc) {
for (auto *ident : call_proc.result_identifiers_) {
if (HasSymbolLocalScope(ident->name_)) {
if (HasSymbol(ident->name_)) {
throw RedeclareVariableError(ident->name_);
}
ident->MapTo(CreateSymbol(ident->name_, true));
@ -217,7 +209,7 @@ bool SymbolGenerator::PostVisit(CallProcedure &call_proc) {
bool SymbolGenerator::PreVisit(LoadCsv &load_csv) { return false; }
bool SymbolGenerator::PostVisit(LoadCsv &load_csv) {
if (HasSymbolLocalScope(load_csv.row_var_->name_)) {
if (HasSymbol(load_csv.row_var_->name_)) {
throw RedeclareVariableError(load_csv.row_var_->name_);
}
load_csv.row_var_->MapTo(CreateSymbol(load_csv.row_var_->name_, true));
@ -265,7 +257,7 @@ bool SymbolGenerator::PostVisit(Merge &) {
bool SymbolGenerator::PostVisit(Unwind &unwind) {
const auto &name = unwind.named_expression_->name_;
if (HasSymbolLocalScope(name)) {
if (HasSymbol(name)) {
throw RedeclareVariableError(name);
}
unwind.named_expression_->MapTo(CreateSymbol(name, true));
@ -282,7 +274,7 @@ bool SymbolGenerator::PostVisit(Match &) {
// Check variables in property maps after visiting Match, so that they can
// reference symbols out of bind order.
for (auto &ident : scope.identifiers_in_match) {
if (!HasSymbolLocalScope(ident->name_) && !ConsumePredefinedIdentifier(ident->name_))
if (!HasSymbol(ident->name_) && !ConsumePredefinedIdentifier(ident->name_))
throw UnboundVariableError(ident->name_);
ident->MapTo(scope.symbols[ident->name_]);
}
@ -314,7 +306,7 @@ SymbolGenerator::ReturnType SymbolGenerator::Visit(Identifier &ident) {
if (scope.in_pattern && !(scope.in_node_atom || scope.visiting_edge)) {
// If we are in the pattern, and outside of a node or an edge, the
// identifier is the pattern name.
symbol = GetOrCreateSymbolLocalScope(ident.name_, ident.user_declared_, Symbol::Type::PATH);
symbol = GetOrCreateSymbol(ident.name_, ident.user_declared_, Symbol::Type::PATH);
} else if (scope.in_pattern && scope.in_pattern_atom_identifier) {
// Patterns used to create nodes and edges cannot redeclare already
// established bindings. Declaration only happens in single node
@ -322,19 +314,19 @@ SymbolGenerator::ReturnType SymbolGenerator::Visit(Identifier &ident) {
// `MATCH (n) CREATE (n)` should throw an error that `n` is already
// declared. While `MATCH (n) CREATE (n) -[:R]-> (n)` is allowed,
// since `n` now references the bound node instead of declaring it.
if ((scope.in_create_node || scope.in_create_edge) && HasSymbolLocalScope(ident.name_)) {
if ((scope.in_create_node || scope.in_create_edge) && HasSymbol(ident.name_)) {
throw RedeclareVariableError(ident.name_);
}
auto type = Symbol::Type::VERTEX;
if (scope.visiting_edge) {
// Edge referencing is not allowed (like in Neo4j):
// `MATCH (n) - [r] -> (n) - [r] -> (n) RETURN r` is not allowed.
if (HasSymbolLocalScope(ident.name_)) {
if (HasSymbol(ident.name_)) {
throw RedeclareVariableError(ident.name_);
}
type = scope.visiting_edge->IsVariable() ? Symbol::Type::EDGE_LIST : Symbol::Type::EDGE;
}
symbol = GetOrCreateSymbolLocalScope(ident.name_, ident.user_declared_, type);
symbol = GetOrCreateSymbol(ident.name_, ident.user_declared_, type);
} else if (scope.in_pattern && !scope.in_pattern_atom_identifier && scope.in_match) {
if (scope.in_edge_range && scope.visiting_edge->identifier_->name_ == ident.name_) {
// Prevent variable path bounds to reference the identifier which is bound
@ -461,7 +453,7 @@ bool SymbolGenerator::PreVisit(NodeAtom &node_atom) {
auto &scope = scopes_.back();
auto check_node_semantic = [&node_atom, &scope, this](const bool props_or_labels) {
const auto &node_name = node_atom.identifier_->name_;
if ((scope.in_create || scope.in_merge) && props_or_labels && HasSymbolLocalScope(node_name)) {
if ((scope.in_create || scope.in_merge) && props_or_labels && HasSymbol(node_name)) {
throw SemanticException("Cannot create node '" + node_name +
"' with labels or properties, because it is already declared.");
}
@ -555,11 +547,11 @@ bool SymbolGenerator::PreVisit(EdgeAtom &edge_atom) {
edge_atom.identifier_->Accept(*this);
scope.in_pattern_atom_identifier = false;
if (edge_atom.total_weight_) {
if (HasSymbolLocalScope(edge_atom.total_weight_->name_)) {
if (HasSymbol(edge_atom.total_weight_->name_)) {
throw RedeclareVariableError(edge_atom.total_weight_->name_);
}
edge_atom.total_weight_->MapTo(GetOrCreateSymbolLocalScope(
edge_atom.total_weight_->name_, edge_atom.total_weight_->user_declared_, Symbol::Type::NUMBER));
edge_atom.total_weight_->MapTo(GetOrCreateSymbol(edge_atom.total_weight_->name_,
edge_atom.total_weight_->user_declared_, Symbol::Type::NUMBER));
}
return false;
}
@ -602,10 +594,6 @@ bool SymbolGenerator::HasSymbol(const std::string &name) const {
return std::ranges::any_of(scopes_, [&name](const auto &scope) { return scope.symbols.contains(name); });
}
bool SymbolGenerator::HasSymbolLocalScope(const std::string &name) const {
return scopes_.back().symbols.contains(name);
}
bool SymbolGenerator::ConsumePredefinedIdentifier(const std::string &name) {
auto it = predefined_identifiers_.find(name);

4
src/query/frontend/semantic/symbol_generator.hpp
View File

@ -1,4 +1,4 @@
// Copyright 2022 Memgraph Ltd.
// Copyright 2023 Memgraph Ltd.
//
// Use of this software is governed by the Business Source License
// included in the file licenses/BSL.txt; by using this file, you agree to be bound by the terms of the Business Source
@ -134,7 +134,6 @@ class SymbolGenerator : public HierarchicalTreeVisitor {
static std::optional<Symbol> FindSymbolInScope(const std::string &name, const Scope &scope, Symbol::Type type);
bool HasSymbol(const std::string &name) const;
bool HasSymbolLocalScope(const std::string &name) const;
// @return true if it added a predefined identifier with that name
bool ConsumePredefinedIdentifier(const std::string &name);
@ -147,7 +146,6 @@ class SymbolGenerator : public HierarchicalTreeVisitor {
auto GetOrCreateSymbol(const std::string &name, bool user_declared, Symbol::Type type = Symbol::Type::ANY);
// Returns the symbol by name. If the mapping already exists, checks if the
// types match. Otherwise, returns a new symbol.
auto GetOrCreateSymbolLocalScope(const std::string &name, bool user_declared, Symbol::Type type = Symbol::Type::ANY);
void VisitReturnBody(ReturnBody &body, Where *where = nullptr);

42
tests/gql_behave/tests/memgraph_V1/features/foreach.feature
View File

@ -66,29 +66,13 @@ Feature: Foreach
| 4 |
And no side effects
Scenario: Foreach shadowing in create
Given an empty graph
And having executed
"""
FOREACH (i IN [1] | FOREACH (j IN [3,4] | CREATE (i {prop: j})));
"""
When executing query:
"""
MATCH (n) RETURN n.prop
"""
Then the result should be:
| n.prop |
| 3 |
| 4 |
And no side effects
Scenario: Foreach set
Given an empty graph
And having executed
"""
CREATE (n1 { marked: false })-[:RELATES]->(n2 { marked: false })
"""
And having executed
And having executed
"""
MATCH p=(n1)-[*]->(n2)
FOREACH (n IN nodes(p) | SET n.marked = true)
@ -110,7 +94,7 @@ Feature: Foreach
"""
CREATE (n1 { marked: false })-[:RELATES]->(n2 { marked: false })
"""
And having executed
And having executed
"""
MATCH p=(n1)-[*]->(n2)
FOREACH (n IN nodes(p) | REMOVE n.marked)
@ -132,7 +116,7 @@ Feature: Foreach
"""
CREATE (n1 { marked: false })-[:RELATES]->(n2 { marked: false })
"""
And having executed
And having executed
"""
MATCH p=(n1)-[*]->(n2)
FOREACH (n IN nodes(p) | DETACH delete n)
@ -148,7 +132,7 @@ Feature: Foreach
Scenario: Foreach merge
Given an empty graph
And having executed
And having executed
"""
FOREACH (i IN [1, 2, 3] | MERGE (n { age : i }))
"""
@ -166,7 +150,7 @@ Feature: Foreach
Scenario: Foreach nested
Given an empty graph
And having executed
And having executed
"""
FOREACH (i IN [1, 2, 3] | FOREACH( j IN [1] | CREATE (k { prop : j })))
"""
@ -183,11 +167,11 @@ Feature: Foreach
Scenario: Foreach multiple update clauses
Given an empty graph
And having executed
And having executed
"""
CREATE (n1 { marked1: false, marked2: false })-[:RELATES]->(n2 { marked1: false, marked2: false })
"""
And having executed
And having executed
"""
MATCH p=(n1)-[*]->(n2)
FOREACH (n IN nodes(p) | SET n.marked1 = true SET n.marked2 = true)
@ -205,11 +189,11 @@ Feature: Foreach
Scenario: Foreach multiple nested update clauses
Given an empty graph
And having executed
And having executed
"""
CREATE (n1 { marked1: false, marked2: false })-[:RELATES]->(n2 { marked1: false, marked2: false })
"""
And having executed
And having executed
"""
MATCH p=(n1)-[*]->(n2)
FOREACH (n IN nodes(p) | FOREACH (j IN [1] | SET n.marked1 = true SET n.marked2 = true))
@ -227,7 +211,7 @@ Feature: Foreach
Scenario: Foreach match foreach return
Given an empty graph
And having executed
And having executed
"""
CREATE (n {prop: [[], [1,2]]});
"""
@ -242,7 +226,7 @@ Feature: Foreach
Scenario: Foreach on null value
Given an empty graph
And having executed
And having executed
"""
CREATE (n);
"""
@ -254,9 +238,9 @@ Feature: Foreach
| |
And no side effects
Scenario: Foreach nested merge
Scenario: Foreach nested merge
Given an empty graph
And having executed
And having executed
"""
FOREACH(i in [1, 2, 3] | foreach(j in [1] | MERGE (n { age : i })));
"""

2
tests/unit/query_semantic.cpp
View File

@ -1,4 +1,4 @@
// Copyright 2022 Memgraph Ltd.
// Copyright 2023 Memgraph Ltd.
//
// Use of this software is governed by the Business Source License
// included in the file licenses/BSL.txt; by using this file, you agree to be bound by the terms of the Business Source