Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

edges are not bound-able #498

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

edges are not bound-able #498

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
4e3fa07
edges are not boundable
swilly22 Nov 6, 2023
c613605
Merge branch 'master' into edge-reuse-validation
swilly22 Apr 4, 2024
c7d7a3e
Merge branch 'master' into edge-reuse-validation
swilly22 Apr 4, 2024
a5b3db3
Merge branch 'master' into edge-reuse-validation
gkorland May 26, 2024
c7c198a
removed unnecessary alias type lookup
swilly22 Jun 4, 2024
bb22b30
changed error msg, reused existing one
swilly22 Jun 4, 2024
aaf7ba9
Merge branch 'master' into edge-reuse-validation
swilly22 Jun 4, 2024
0be9ca8
Merge branch 'master' into edge-reuse-validation
swilly22 Jun 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
119 changes: 28 additions & 91 deletions src/ast/ast_validations.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,7 @@ static AST_Validation _ValidateMultiHopTraversal
end = AST_ParseIntegerNode(range_end);
}

// Validate specified range
// validate specified range
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure proper handling of negative range values in _ValidateMultiHopTraversal.

+ // Ensure start and end are non-negative
+ if(start < 0 || end < 0) {
+     ErrorCtx_SetError(EMSG_VAR_LEN_INVALID_RANGE);
+     return AST_INVALID;
+ }

The function _ValidateMultiHopTraversal currently checks if the start of the range is greater than the end, but it does not explicitly check for negative values. In Cypher, ranges should be non-negative. Adding a check for negative values would prevent potential issues with range specifications.

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
// validate specified range
// validate specified range
+ // Ensure start and end are non-negative
+ if(start < 0 || end < 0) {
+ ErrorCtx_SetError(EMSG_VAR_LEN_INVALID_RANGE);
+ return AST_INVALID;
+ }

if(start > end) {
ErrorCtx_SetError(EMSG_VAR_LEN_INVALID_RANGE);
return AST_INVALID;
Expand All @@ -259,44 +259,6 @@ static AST_Validation _ValidateMultiHopTraversal
return AST_VALID;
}

// Verify that MERGE doesn't redeclare bound relations, that one reltype is specified for unbound relations,
// and that the entity is not a variable length pattern
static AST_Validation _ValidateMergeRelation
(
const cypher_astnode_t *entity, // ast-node (rel-pattern)
validations_ctx *vctx
) {
// Verify that this is not a variable length relationship
const cypher_astnode_t *range = cypher_ast_rel_pattern_get_varlength(entity);
if(range) {
ErrorCtx_SetError(EMSG_VAR_LEN, "MERGE");
return AST_INVALID;
}

const cypher_astnode_t *identifier = cypher_ast_rel_pattern_get_identifier(entity);
const char *alias = NULL;
if(identifier) {
alias = cypher_ast_identifier_get_name(identifier);
// verify that we're not redeclaring a bound variable
if(_IdentifiersFind(vctx, alias) != raxNotFound) {
ErrorCtx_SetError(EMSG_REDECLARE, "variable", alias, "MERGE");
return AST_INVALID;
}
}

// Exactly one reltype should be specified for the introduced edge
uint reltype_count = cypher_ast_rel_pattern_nreltypes(entity);
if(reltype_count != 1) {
ErrorCtx_SetError(EMSG_ONE_RELATIONSHIP_TYPE, "MERGE");
return AST_INVALID;
}

// We don't need to validate the MERGE edge's direction, as an undirected edge
// in MERGE should result a single outgoing edge to be created.

return AST_VALID;
}

// Verify that MERGE does not introduce labels or properties to bound nodes
static AST_Validation _ValidateMergeNode
(
Expand Down Expand Up @@ -328,24 +290,6 @@ static AST_Validation _ValidateMergeNode
return AST_VALID;
}

// validate that the relation alias of an edge is not bound
static AST_Validation _ValidateCreateRelation
(
const cypher_astnode_t *entity, // ast-node
validations_ctx *vctx
) {
const cypher_astnode_t *identifier = cypher_ast_rel_pattern_get_identifier(entity);
if(identifier) {
const char *alias = cypher_ast_identifier_get_name(identifier);
if(_IdentifiersFind(vctx, alias) != raxNotFound) {
ErrorCtx_SetError(EMSG_REDECLARE, "variable", alias, "CREATE");
return AST_INVALID;
}
}

return AST_VALID;
}

// validate each entity referenced in a single path of a CREATE clause
static AST_Validation _Validate_CREATE_Entities
(
Expand Down Expand Up @@ -792,67 +736,60 @@ static VISITOR_STRATEGY _Validate_rel_pattern
return VISITOR_CONTINUE;
}

uint reltype_count = cypher_ast_rel_pattern_nreltypes(n);
enum cypher_rel_direction dir = cypher_ast_rel_pattern_get_direction(n);
const cypher_astnode_t *range = cypher_ast_rel_pattern_get_varlength(n);
if(vctx->clause == CYPHER_AST_CREATE) {
// validate that the relation alias is not bound
if(_ValidateCreateRelation(n, vctx) == AST_INVALID) {
return VISITOR_BREAK;
}

// Validate that each relation has exactly one type
uint reltype_count = cypher_ast_rel_pattern_nreltypes(n);
// clause name used for error reporting
const char *clause_name = (vctx->clause == CYPHER_AST_CREATE) ?
"CREATE" :
"MERGE";

// extra validation when in a CREATE / MERGE clause
if(vctx->clause == CYPHER_AST_CREATE || vctx->clause == CYPHER_AST_MERGE) {

// validate that each relation has exactly one type
if(reltype_count != 1) {
ErrorCtx_SetError(EMSG_ONE_RELATIONSHIP_TYPE, "CREATE");
// CREATE ()-[:A:B]->()
ErrorCtx_SetError(EMSG_ONE_RELATIONSHIP_TYPE, clause_name);
return VISITOR_BREAK;
}

// Validate that each relation being created is directed
if(cypher_ast_rel_pattern_get_direction(n) == CYPHER_REL_BIDIRECTIONAL) {
// validate that each relation being created is directed
// CREATE ()-[]-()
if(dir == CYPHER_REL_BIDIRECTIONAL) {
ErrorCtx_SetError(EMSG_CREATE_DIRECTED_RELATIONSHIP);
return VISITOR_BREAK;
}

// Validate that each relation being created is not variable length relationship
// validate that each relation being created is not of variable length
// CREATE ()-[*2..4]->()
if(range) {
ErrorCtx_SetError(EMSG_VAR_LEN, "CREATE");
ErrorCtx_SetError(EMSG_VAR_LEN, clause_name);
return VISITOR_BREAK;
}
}

if(_ValidateInlinedProperties(cypher_ast_rel_pattern_get_properties(n)) == AST_INVALID) {
return VISITOR_BREAK;
}

if(vctx->clause == CYPHER_AST_MERGE &&
_ValidateMergeRelation(n, vctx) == AST_INVALID) {
const cypher_astnode_t *props = cypher_ast_rel_pattern_get_properties(n);
if(_ValidateInlinedProperties(props) == AST_INVALID) {
return VISITOR_BREAK;
}

const cypher_astnode_t *alias_node = cypher_ast_rel_pattern_get_identifier(n);
if(!alias_node && !range) {
return VISITOR_RECURSE; // Skip unaliased, single-hop entities.
return VISITOR_RECURSE; // skip unaliased, single-hop entities
}

// If this is a multi-hop traversal, validate it accordingly
// if this is a multi-hop traversal, validate it accordingly
if(range && _ValidateMultiHopTraversal(n, range) == AST_INVALID) {
return VISITOR_BREAK;
}

if(alias_node) {
const char *alias = cypher_ast_identifier_get_name(alias_node);
void *alias_type = _IdentifiersFind(vctx, alias);
if(alias_type == raxNotFound) {
_IdentifierAdd(vctx, alias, (void*)T_EDGE);
return VISITOR_RECURSE;
}

if(alias_type != (void *)T_EDGE && alias_type != NULL) {
ErrorCtx_SetError(EMSG_SAME_ALIAS_NODE_RELATIONSHIP, alias);
return VISITOR_BREAK;
}

if(vctx->clause == CYPHER_AST_MATCH && alias_type != NULL) {
ErrorCtx_SetError(EMSG_SAME_ALIAS_MULTIPLE_PATTERNS, alias);
// edge can not be redeclared
if(_IdentifierAdd(vctx, alias, (void*)T_EDGE) == 0) {
ErrorCtx_SetError(EMSG_REDECLARE, "edge", alias, clause_name);
return VISITOR_BREAK;
}
}
Expand Down Expand Up @@ -1737,7 +1674,7 @@ static VISITOR_STRATEGY _Validate_CREATE_Clause

// fail on duplicate identifier
if(_IdentifierAdd(vctx, alias, (void*)t) == 0 && t == T_EDGE) {
ErrorCtx_SetError(EMSG_VAIABLE_ALREADY_DECLARED, alias);
ErrorCtx_SetError(EMSG_REDECLARE, "edge", alias, "CREATE");
res = VISITOR_BREAK;
break;
}
Expand Down
32 changes: 31 additions & 1 deletion tests/flow/test_bound_variables.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

class testBoundVariables(FlowTestsBase):
def __init__(self):

self.env, self.db = Env()
self.graph = self.db.select_graph(GRAPH_ID)
self.populate_graph()
Expand All @@ -20,6 +21,7 @@ def populate_graph(self):
for idx, v in enumerate(node_props):
node = Node(alias=f"n_{idx}", labels="L", properties={"val": v})
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure that Node and Edge are defined or imported explicitly.

+ from graph_entities import Node, Edge

Also applies to: 27-28

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
node = Node(alias=f"n_{idx}", labels="L", properties={"val": v})
+ from graph_entities import Node, Edge
node = Node(alias=f"n_{idx}", labels="L", properties={"val": v})
Tools
Ruff

22-22: Node may be undefined, or defined from star imports (F405)

nodes.append(node)

nodes_str = [str(n) for n in nodes]

e0 = Edge(nodes[0], "E", nodes[1])
Expand All @@ -33,6 +35,7 @@ def test01_with_projected_entity(self):

# Verify that this query does not generate a Cartesian product.
execution_plan = str(self.graph.explain(query))

self.env.assertNotIn('Cartesian Product', execution_plan)

# Verify results.
Expand All @@ -44,6 +47,7 @@ def test02_match_create_bound_variable(self):
# (v1)-[:E]->(v2)-[:E]->(v3)-[:e]->(v4)
query = """MATCH (a:L {val: 'v3'}) CREATE (a)-[:E]->(b:L {val: 'v4'}) RETURN b.val"""
actual_result = self.graph.query(query)

expected_result = [['v4']]
self.env.assertEquals(actual_result.result_set, expected_result)
self.env.assertEquals(actual_result.relationships_created, 1)
Expand All @@ -53,10 +57,12 @@ def test03_procedure_match_bound_variable(self):
# Create a full-text index.
create_node_fulltext_index(self.graph, "L", "val", sync=True)


# Project the result of scanning this index into a MATCH pattern.
query = """CALL db.idx.fulltext.queryNodes('L', 'v1') YIELD node MATCH (node)-[]->(b) RETURN b.val"""
# Verify that execution begins at the procedure call and proceeds into the traversals.
execution_plan = str(self.graph.explain(query))

# For the moment, we'll just verify that ProcedureCall appears later in the plan than
# its parent, Conditional Traverse.
traverse_idx = execution_plan.index("Conditional Traverse")
Expand All @@ -70,10 +76,12 @@ def test03_procedure_match_bound_variable(self):

def test04_projected_scanned_entity(self):
query = """MATCH (a:L {val: 'v1'}) WITH a MATCH (a), (b {val: 'v2'}) RETURN a.val, b.val"""

actual_result = self.graph.query(query)

# Verify that this query generates exactly 2 scan ops.
execution_plan = str(self.graph.explain(query))

self.env.assertEquals(2, execution_plan.count('Scan'))

# Verify results.
Expand All @@ -99,7 +107,29 @@ def test06_override_bound_with_label(self):
res = self.graph.query("CREATE (:N)")
self.env.assertEquals(res.nodes_created, 1)

res = self.graph.query("MATCH(n:N) WITH n MATCH (n:X) RETURN n")
res = self.graph.query("MATCH (n:N) WITH n MATCH (n:X) RETURN n")

# make sure no nodes were returned
self.env.assertEquals(len(res.result_set), 0)

def test07_bound_edges(self):
# edges can only be declared once
# re-declaring a variable as an edge is forbidden

queries = ["MATCH ()-[e]->()-[e]->() RETURN *",
"MATCH ()-[e]->() MATCH ()<-[e]-() RETURN *",
"WITH NULL AS e MATCH ()-[e]->() RETURN *",
"MATCH ()-[e]->() WITH e MATCH ()-[e]->() RETURN *",
"MATCH ()-[e]->() MERGE ()-[e:R]->() RETURN *",
"WITH NULL AS e MERGE ()-[e:R]->() RETURN *",
"MERGE ()-[e:R]->() MERGE ()-[e:R]->()",
"MATCH ()-[e]->() WHERE ()-[e]->() RETURN *"]

for q in queries:
try:
res = self.graph.query(q)
# should not reach this point
self.env.assertFalse(True)
except Exception as e:
self.env.assertIn("The bound edge 'e' can't be redeclared in a MERGE clause", str(e))

5 changes: 2 additions & 3 deletions tests/flow/test_create_clause.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,14 +50,13 @@ def test01_create_dependency(self):

def test02_edge_reuse(self):
# bound edges can not be used in a CREATE clause

q = "CREATE ()-[e:R]->()-[e:R]->()"
try:
self.g.query(q)
# should not reach this point
self.env.assertTrue(False)
except Exception as e:
self.env.assertTrue("Variable `e` already declared" in str(e))
self.env.assertTrue("The bound edge 'e' can't be redeclared in a CREATE clause" in str(e))

queries = ["MATCH ()-[e:R]->() CREATE ()-[e:R]->()",
"CREATE ()-[e:R]->() CREATE ()-[e:R]->()"]
Expand All @@ -68,5 +67,5 @@ def test02_edge_reuse(self):
# should not reach this point
self.env.assertTrue(False)
except Exception as e:
self.env.assertTrue("The bound variable 'e' can't be redeclared in a CREATE clause" in str(e))
self.env.assertTrue("The bound edge 'e' can't be redeclared in a CREATE clause" in str(e))

11 changes: 6 additions & 5 deletions tests/flow/test_graph_create.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,21 +134,22 @@ def test09_create_use_alias_in_many_clauses(self):
self.env.assertEquals(result.relationships_created, 2)

queries = ["CREATE (n1)-[r:Rel1]->(n2) CREATE (n2)-[r:Rel1]->(n1)",
"CREATE (n1)-[r:Rel1]->(n2) CREATE (n2)-[r2:Rel1]->(n3), (n3)-[r:Rel1]->(n2)",
"MATCH (r) CREATE (r)"]
"CREATE (n1)-[r:Rel1]->(n2), (n2)-[r:Rel1]->(n1)",
"CREATE (n1)-[r:Rel1]->(n2) CREATE (n2)-[r2:Rel1]->(n3), (n3)-[r:Rel1]->(n2)"]

for query in queries:
try:
self.graph.query(query)
self.env.assertTrue(False)
except redis.exceptions.ResponseError as e:
self.env.assertContains("The bound variable 'r' can't be redeclared in a CREATE clause", str(e))
self.env.assertContains("The bound edge 'r' can't be redeclared in a CREATE clause", str(e))

query = "CREATE (n1)-[r:Rel1]->(n2), (n2)-[r:Rel1]->(n1)"
query = "MATCH (r) CREATE (r)"
try:
self.graph.query(query)
self.env.assertTrue(False)
except redis.exceptions.ResponseError as e:
self.env.assertContains("Variable `r` already declared", str(e))
self.env.assertContains("The bound variable 'r' can't be redeclared in a CREATE clause", str(e))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refine the error message check to ensure it precisely matches the new standard error message for variable redeclaration.

- self.env.assertContains("The bound variable 'r' can't be redeclared in a CREATE clause", str(e))
+ self.env.assertContains("The bound variable 'r' cannot be redeclared in a CREATE clause", str(e))

Note: This suggestion assumes that the error message standardization also applies to other variable redeclaration scenarios. If not, please disregard this comment.

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
self.env.assertContains("The bound variable 'r' can't be redeclared in a CREATE clause", str(e))
self.env.assertContains("The bound variable 'r' cannot be redeclared in a CREATE clause", str(e))


# test creating queries with matching relationship type :R|R
# the results can't report duplicates
Expand Down
16 changes: 7 additions & 9 deletions tests/flow/test_null_handling.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,15 +96,13 @@ def test07_null_graph_entity_inputs(self):
expected_result = []
self.env.assertEquals(actual_result.result_set, expected_result)

query = """WITH NULL AS e MATCH (a:L)-[e]->(b) RETURN a, e, b"""
plan = str(self.graph.explain(query))
# Verify that we are performing a scan and traversal.
self.env.assertIn("Label Scan", plan)
self.env.assertIn("Conditional Traverse", plan)
actual_result = self.graph.query(query)
# Expect no results.
expected_result = []
self.env.assertEquals(actual_result.result_set, expected_result)
try:
query = """WITH NULL AS e MATCH (a:L)-[e]->(b) RETURN a, e, b"""
plan = self.graph.explain(query)
# not expecting to reach this point
self.env.assertTrue(False)
except redis.exceptions.ResponseError as e:
self.env.assertContains("The bound edge 'e' can't be redeclared in a MERGE clause", str(e))

# ValueHashJoin ops should not treat null values as equal.
def test08_null_value_hash_join(self):
Expand Down
Loading