-
Notifications
You must be signed in to change notification settings - Fork 230
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.
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
Call subquery validate return #3107
base: master
Are you sure you want to change the base?
Changes from 18 commits
428d42c
0daca85
d950f8a
e084f48
c3d596c
6deb8e2
f56d1dc
0eb28d5
ada954b
87e4298
63ffebe
4b3190c
eb3be08
052e11a
4487073
6e76e74
ac8ce42
44bfc19
23fa13d
5c3e536
8361b4a
aae0e80
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
+31 −0 | lib/src/ast_projection.c | |
+13 −0 | lib/src/cypher-parser.h.in |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -580,6 +580,26 @@ bool AST_ClauseContainsAggregation | |
return aggregated; | ||
} | ||
|
||
const char *AST_GetProjectionAlias | ||
( | ||
const cypher_astnode_t* projection | ||
) { | ||
ASSERT(cypher_astnode_type(projection) == CYPHER_AST_PROJECTION); | ||
|
||
const char *alias = NULL; | ||
const cypher_astnode_t *ast_alias = | ||
cypher_ast_projection_get_alias(projection); | ||
|
||
if(ast_alias != NULL) { | ||
alias = cypher_ast_identifier_get_name(ast_alias); | ||
} else { | ||
const cypher_astnode_t *exp = | ||
cypher_ast_projection_get_expression(projection); | ||
alias = cypher_ast_identifier_get_name(exp); | ||
} | ||
return alias; | ||
} | ||
|
||
const char **AST_BuildReturnColumnNames | ||
( | ||
const cypher_astnode_t *return_clause | ||
|
@@ -591,12 +611,12 @@ const char **AST_BuildReturnColumnNames | |
uint projection_count = cypher_ast_return_nprojections(return_clause); | ||
const char **columns = array_new(const char *, projection_count); | ||
for(uint i = 0; i < projection_count; i++) { | ||
const cypher_astnode_t *projection = cypher_ast_return_get_projection(return_clause, i); | ||
const cypher_astnode_t *ast_alias = cypher_ast_projection_get_alias(projection); | ||
// If the projection was not aliased, the projection itself is an identifier. | ||
if(ast_alias == NULL) ast_alias = cypher_ast_projection_get_expression(projection); | ||
const char *alias = cypher_ast_identifier_get_name(ast_alias); | ||
array_append(columns, alias); | ||
const cypher_astnode_t *projection = | ||
cypher_ast_return_get_projection(return_clause, i); | ||
const char *alias = AST_GetProjectionAlias(projection); | ||
if(alias != NULL) { | ||
array_append(columns, alias); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK. Condition removed in 23fa13d |
||
} | ||
|
||
return columns; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -212,6 +212,20 @@ bool AST_ClauseContainsAggregation | |
const cypher_astnode_t *clause | ||
); | ||
|
||
// returns the alias of a projection | ||
const char *AST_GetProjectionAlias | ||
( | ||
const cypher_astnode_t* projection | ||
); | ||
|
||
// returns the alias of a projection | ||
// the alias will be NULL in the case of an unaliased non-identifier projection | ||
// returned in a Call {} clause | ||
const char *AST_GetSubqueryProjectionAlias | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Couldn't find There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't need this function. Its definition was deleted in 23fa13d. Thanks |
||
( | ||
const cypher_astnode_t* projection | ||
); | ||
|
||
raz-mon marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// collect the aliases from a RETURN clause to populate ResultSet column names | ||
const char **AST_BuildReturnColumnNames | ||
( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1015,14 +1015,9 @@ static AST_Validation _ValidateUnion_Clauses | |
const char *projections[proj_count]; | ||
|
||
for(uint j = 0; j < proj_count; j++) { | ||
const cypher_astnode_t *proj = cypher_ast_return_get_projection(return_clause, j); | ||
const cypher_astnode_t *alias_node = cypher_ast_projection_get_alias(proj); | ||
if(alias_node == NULL) { | ||
// The projection was not aliased, so the projection itself must be an identifier. | ||
alias_node = cypher_ast_projection_get_expression(proj); | ||
ASSERT(cypher_astnode_type(alias_node) == CYPHER_AST_IDENTIFIER); | ||
} | ||
const char *alias = cypher_ast_identifier_get_name(alias_node); | ||
const cypher_astnode_t *proj = | ||
cypher_ast_return_get_projection(return_clause, j); | ||
const char *alias = AST_GetProjectionAlias(proj); | ||
projections[j] = alias; | ||
} | ||
|
||
|
@@ -1035,15 +1030,11 @@ static AST_Validation _ValidateUnion_Clauses | |
} | ||
|
||
for(uint j = 0; j < proj_count; j++) { | ||
const cypher_astnode_t *proj = cypher_ast_return_get_projection(return_clause, j); | ||
const cypher_astnode_t *alias_node = cypher_ast_projection_get_alias(proj); | ||
if(alias_node == NULL) { | ||
// The projection was not aliased, so the projection itself must be an identifier. | ||
alias_node = cypher_ast_projection_get_expression(proj); | ||
ASSERT(cypher_astnode_type(alias_node) == CYPHER_AST_IDENTIFIER); | ||
} | ||
const char *alias = cypher_ast_identifier_get_name(alias_node); | ||
if(strcmp(projections[j], alias) != 0) { | ||
const cypher_astnode_t *proj = | ||
cypher_ast_return_get_projection(return_clause, j); | ||
const char *alias = AST_GetProjectionAlias(proj); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is it possible for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, it can't return |
||
if(alias == NULL || projections[j] == NULL || | ||
raz-mon marked this conversation as resolved.
Show resolved
Hide resolved
|
||
strcmp(projections[j], alias) != 0) { | ||
ErrorCtx_SetError("All sub queries in a UNION must have the same column names."); | ||
res = AST_INVALID; | ||
goto cleanup; | ||
|
@@ -1349,23 +1340,24 @@ references to outside variables"); | |
for(uint i = 0; i < n_projections; i++) { | ||
const cypher_astnode_t *proj = | ||
cypher_ast_return_get_projection(return_clause, i); | ||
const char *var_name; | ||
const cypher_astnode_t *identifier = | ||
const cypher_astnode_t *alias_node = | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. are we using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, we don't need it. The variable was removed in 23fa13d |
||
cypher_ast_projection_get_alias(proj); | ||
const cypher_astnode_t *exp = | ||
cypher_ast_projection_get_expression(proj); | ||
if(identifier) { | ||
var_name = cypher_ast_identifier_get_name(identifier); | ||
if(exp && | ||
cypher_astnode_type(exp) == CYPHER_AST_IDENTIFIER && | ||
cypher_ast_identifier_get_name(exp)[0] == '@') { | ||
// this is an artificial projection, skip it | ||
continue; | ||
} | ||
} else { | ||
var_name = cypher_ast_identifier_get_name(exp); | ||
|
||
if(cypher_ast_projection_is_aliased(proj) == false) { | ||
ErrorCtx_SetError("Return projection in CALL {} must be aliased"); | ||
return VISITOR_BREAK; | ||
} | ||
|
||
if(exp && | ||
cypher_astnode_type(exp) == CYPHER_AST_IDENTIFIER && | ||
cypher_ast_identifier_get_name(exp)[0] == '@') { | ||
// this is an artificial projection, skip it | ||
continue; | ||
} | ||
|
||
const char *var_name = AST_GetProjectionAlias(proj); | ||
if(!raxTryInsert(vctx->defined_identifiers, | ||
(unsigned char *)var_name, strlen(var_name), NULL, NULL)) { | ||
ErrorCtx_SetError( | ||
|
@@ -1747,13 +1739,12 @@ static VISITOR_STRATEGY _Validate_RETURN_Clause | |
|
||
// introduce bound vars | ||
for(uint i = 0; i < num_return_projections; i ++) { | ||
const cypher_astnode_t *child = cypher_ast_return_get_projection(n, i); | ||
const cypher_astnode_t *alias_node = cypher_ast_projection_get_alias(child); | ||
if(alias_node == NULL) { | ||
continue; | ||
const cypher_astnode_t *proj = cypher_ast_return_get_projection(n, i); | ||
const char *alias = AST_GetProjectionAlias(proj); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In which case There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It can no longer return NULL, the condition is no longer needed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK. The condition was removed. |
||
if(alias != NULL) { | ||
raxInsert(vctx->defined_identifiers, (unsigned char *)alias, | ||
strlen(alias), NULL, NULL); | ||
} | ||
const char *alias = cypher_ast_identifier_get_name(alias_node); | ||
raxInsert(vctx->defined_identifiers, (unsigned char *)alias, strlen(alias), NULL, NULL); | ||
} | ||
|
||
// visit ORDER BY clause | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,30 +42,104 @@ def expect_error(self, query, expected_err_msg): | |
def test01_test_validations(self): | ||
"""Make sure we fail on invalid queries""" | ||
|
||
import_error = "WITH imports in CALL {} must consist of only simple references to outside variables" | ||
match_after_updating = "A WITH clause is required to introduce MATCH after an updating clause" | ||
union_column_name_error = "All sub queries in a UNION must have the same column names." | ||
queries_errors = [ | ||
("WITH 1 AS a CALL {WITH a+1 AS b RETURN b} RETURN b", import_error), | ||
("WITH {a: 1} AS map CALL {WITH map.a AS b RETURN b} RETURN b", import_error), | ||
("WITH [1, 2, 3] AS list CALL {WITH list[0] AS b RETURN b} RETURN b", import_error), | ||
("WITH 'RAZ' AS str CALL {WITH toUpper(str) AS b RETURN b} RETURN b", import_error), | ||
("WITH 1 AS a CALL {WITH a AS b RETURN b} RETURN b", import_error), | ||
("WITH 1 AS a CALL {WITH a LIMIT 5 RETURN a} RETURN a", import_error), | ||
("WITH 1 AS a CALL {WITH a ORDER BY a.v RETURN a} RETURN a", import_error), | ||
("WITH 1 AS a CALL {WITH a WHERE a > 5 RETURN a} RETURN a", import_error), | ||
("WITH 1 AS a CALL {WITH a SKIP 5 RETURN a} RETURN a", import_error), | ||
("WITH true AS a CALL {WITH NOT(a) AS b RETURN b} RETURN b", import_error), | ||
("CALL {CREATE (n:N) MATCH (n:N) RETURN n} RETURN 1", match_after_updating), | ||
("WITH 1 AS a CALL {WITH a CREATE (n:N) MATCH (n:N) RETURN n} RETURN a", match_after_updating), | ||
("CALL {MATCH (n:N) CREATE (n:N2)} RETURN 1 ", "The bound variable 'n' can't be redeclared in a CREATE clause"), | ||
("MATCH (n) CALL {WITH n AS n1 RETURN n1 UNION WITH n RETURN n1} RETURN n, n1", import_error), | ||
("MATCH (n) CALL {WITH n RETURN n AS n1 UNION WITH n AS n1 RETURN n1} RETURN n, n1", import_error), | ||
("CALL {RETURN 1 AS one UNION RETURN 2 AS two} RETURN 1", union_column_name_error), | ||
("MATCH (n) CALL {RETURN 1 AS one UNION RETURN 2 AS two} RETURN 1", union_column_name_error) | ||
# invalid queries: imports that are not simple imports | ||
queries = [ | ||
"WITH 1 AS a CALL {WITH a + 1 AS b RETURN b} RETURN b", | ||
"WITH {a: 1} AS map CALL {WITH map.a AS b RETURN b} RETURN b", | ||
"WITH [1, 2, 3] AS list CALL {WITH list[0] AS b RETURN b} RETURN b", | ||
"WITH 'RAZ' AS str CALL {WITH toUpper(str) AS b RETURN b} RETURN b", | ||
"WITH 1 AS a CALL {WITH a AS b RETURN b} RETURN b", | ||
"WITH 1 AS a CALL {WITH a LIMIT 5 RETURN a} RETURN a", | ||
"WITH 1 AS a CALL {WITH a ORDER BY a.v RETURN a} RETURN a", | ||
"WITH 1 AS a CALL {WITH a WHERE a > 5 RETURN a} RETURN a", | ||
"WITH 1 AS a CALL {WITH a SKIP 5 RETURN a} RETURN a", | ||
"WITH true AS a CALL {WITH NOT(a) AS b RETURN b} RETURN b", | ||
"MATCH (n) CALL {WITH n AS n1 RETURN n1 UNION WITH n RETURN n1} RETURN n, n1", | ||
"MATCH (n) CALL {WITH n RETURN n AS n1 UNION WITH n AS n1 RETURN n1} RETURN n, n1", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what's the issue with this last query ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Non-simple import in the second branch of the |
||
] | ||
expected_error = "WITH imports in CALL {} must consist of only simple references to outside variables" | ||
for query in queries: | ||
self.expect_error(query, expected_error) | ||
|
||
# invalid queries: MATCH without preceding WITH | ||
queries = [ | ||
"CALL {CREATE (n:N) MATCH (n:N) RETURN n} RETURN 1", | ||
"WITH 1 AS a CALL {WITH a CREATE (n:N) MATCH (n:N) RETURN n} RETURN a", | ||
] | ||
expected_error = "A WITH clause is required to introduce MATCH after an updating clause" | ||
for query in queries: | ||
self.expect_error(query, expected_error) | ||
|
||
# invalid queries: redeclaring variable in CREATE | ||
queries = [ | ||
"CALL {MATCH (n:N) CREATE (n:N2)} RETURN 1 ", | ||
] | ||
expected_error = "The bound variable 'n' can't be redeclared in a CREATE clause" | ||
for query in queries: | ||
self.expect_error(query, expected_error) | ||
|
||
# invalid queries: UNION column names | ||
queries = [ | ||
# different column names | ||
""" | ||
CALL { | ||
RETURN 1 AS one | ||
UNION | ||
RETURN 2 AS two | ||
} | ||
RETURN 1 | ||
""", | ||
""" | ||
MATCH (n) | ||
CALL { | ||
RETURN 1 AS one | ||
UNION | ||
RETURN n | ||
} RETURN 1 | ||
""", | ||
# missing column names | ||
""" | ||
CALL { | ||
RETURN 1 | ||
UNION | ||
RETURN 2 AS two | ||
} RETURN 1 | ||
""", | ||
] | ||
expected_error = "All sub queries in a UNION must have the same column names" | ||
for query in queries: | ||
self.expect_error(query, expected_error) | ||
|
||
# invalid queries: wrong number of UNION clauses | ||
queries = [ | ||
"CALL {WITH 1 UNION RETURN 1} RETURN 0" | ||
] | ||
expected_error = "Found 1 UNION clauses but only 1 RETURN clauses" | ||
for query in queries: | ||
self.expect_error(query, expected_error) | ||
|
||
# invalid queries: subquery with invalid termination | ||
queries = [ | ||
"CALL {MATCH (n)} RETURN 0", | ||
"CALL {RETURN 1 UNION WITH 1} RETURN 0" | ||
] | ||
for query, err in queries_errors: | ||
self.expect_error(query, err) | ||
expected_error = "Query cannot conclude with" | ||
for query in queries: | ||
self.expect_error(query, expected_error) | ||
|
||
# invalid queries: returning literals inside CALL{} must be aliased | ||
queries = [ | ||
"CALL {RETURN 1} RETURN 0", | ||
"CALL {RETURN 'a'} RETURN 0", | ||
"CALL {RETURN 5.7} RETURN 0", | ||
"CALL {RETURN 1+2} RETURN 0", | ||
"CALL {RETURN true} RETURN 0", | ||
"CALL {RETURN sum(0)} RETURN 0", | ||
"CALL {MATCH (n) RETURN n.x} RETURN 0", | ||
"CALL {MATCH p=(n) RETURN nodes(p)[0]} RETURN 0", | ||
] | ||
for query in queries: | ||
self.expect_error(query, "must be aliased") | ||
raz-mon marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
# invalid queries: import an undefined identifier | ||
queries = [ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please copy the comments from the header file to this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. Done in 23fa13d