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.

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

Open
wants to merge 22 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
428d42c
Validate CALL{ RETURN literal}
nafraf Jun 21, 2023
0daca85
Update deps/libcypher-parser
nafraf Jun 21, 2023
d950f8a
Merge branch 'master' into call_subquery_validate_return
nafraf Jun 21, 2023
e084f48
Continue if alias is null
nafraf Jun 21, 2023
c3d596c
Add function AST_GetProjectionAlias()
nafraf Jun 22, 2023
6deb8e2
Merge branch 'master' into call_subquery_validate_return
nafraf Jun 22, 2023
f56d1dc
AST_GetProjectionAlias in _Validate_call_subquery
nafraf Jun 22, 2023
0eb28d5
WIP: Print time taken in test_AbortRunningTask()
nafraf Jun 22, 2023
ada954b
Revert changes done in test_cron.c
nafraf Jun 22, 2023
87e4298
Comments corrections
nafraf Jun 25, 2023
63ffebe
Merge branch 'master' into call_subquery_validate_return
nafraf Jun 26, 2023
4b3190c
Merge branch 'master' into call_subquery_validate_return
nafraf Jun 26, 2023
eb3be08
Merge branch 'master' into call_subquery_validate_return
nafraf Jun 26, 2023
052e11a
Merge branch 'master' into call_subquery_validate_return
nafraf Jun 27, 2023
4487073
Merge branch 'master' into call_subquery_validate_return
nafraf Jun 28, 2023
6e76e74
Add function AST_GetSubqueryProjectionAlias()
nafraf Jun 28, 2023
ac8ce42
Merge branch 'master' into call_subquery_validate_return
nafraf Jun 29, 2023
44bfc19
Use cypher_ast_projection_is_aliased()
nafraf Jun 29, 2023
23fa13d
Remove unneeded validations
nafraf Jul 3, 2023
5c3e536
Use AST_GetProjectionAlias() in AST_BuildCallColumnNames()
nafraf Jul 3, 2023
8361b4a
Revert removing include
nafraf Jul 3, 2023
aae0e80
Merge branch 'master' into call_subquery_validate_return
nafraf Jul 13, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion deps/libcypher-parser
42 changes: 36 additions & 6 deletions src/ast/ast.c
Original file line number Diff line number Diff line change
Expand Up @@ -580,6 +580,34 @@ bool AST_ClauseContainsAggregation
return aggregated;
}

const char *AST_GetProjectionAlias
Copy link
Collaborator

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. Done in 23fa13d

(
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 {
// if the projection was not aliased:
// a) if it is an identifier, the alias will be its name
// b) if it is not an identifier, the alias will be NULL
// the second case occurs when unaliased literals are returned by a
// call subquery: CALL {RETURN 1}
const cypher_astnode_t *exp =
cypher_ast_projection_get_expression(projection);
cypher_astnode_type_t type = cypher_astnode_type(exp);
if(type == CYPHER_AST_IDENTIFIER) {
alias = cypher_ast_identifier_get_name(exp);
}
raz-mon marked this conversation as resolved.
Show resolved Hide resolved
}
return alias;
}

const char **AST_BuildReturnColumnNames
(
const cypher_astnode_t *return_clause
Expand All @@ -591,12 +619,14 @@ 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 cypher_astnode_t *ast_alias =
cypher_ast_projection_get_alias(projection);
const char *alias = AST_GetProjectionAlias(projection);
if(alias != NULL) {
array_append(columns, alias);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems like AST_GetProjectionAlias can't return NULL if my understanding is correct please remove this condition.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. Condition removed in 23fa13d

}

return columns;
Expand Down
8 changes: 8 additions & 0 deletions src/ast/ast.h
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,14 @@ bool AST_ClauseContainsAggregation
const cypher_astnode_t *clause
);

// returns the alias of the projection
// the alias will be NULL in the case of unaliased non-identifiers expressions
// returned by call subquery
raz-mon marked this conversation as resolved.
Show resolved Hide resolved
const char *AST_GetProjectionAlias
(
const cypher_astnode_t* projection
);

// collect the aliases from a RETURN clause to populate ResultSet column names
const char **AST_BuildReturnColumnNames
(
Expand Down
49 changes: 20 additions & 29 deletions src/ast/ast_validations.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand All @@ -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);
Copy link
Collaborator

Choose a reason for hiding this comment

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

is it possible for AST_GetProjectionAlias to return NULL ?

Copy link
Contributor Author

@nafraf nafraf Jul 3, 2023

Choose a reason for hiding this comment

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

No, it can't return NULL. The unneeded comparison was removed from if(alias == NULL || ...

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;
Expand Down Expand Up @@ -1349,21 +1340,22 @@ 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 =
Copy link
Collaborator

Choose a reason for hiding this comment

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

are we using alias_node ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, we don't need it. The variable was removed in 23fa13d
Thanks.

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);
const char *var_name = AST_GetProjectionAlias(proj);

if(var_name == NULL) {
ErrorCtx_SetError("Return projection in CALL {} must be aliased");
return VISITOR_BREAK;
} else {
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(!raxTryInsert(vctx->defined_identifiers,
Expand Down Expand Up @@ -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);
Copy link
Collaborator

Choose a reason for hiding this comment

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

In which case AST_GetProjectionAlias returns NULL ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It can no longer return NULL, the condition is no longer needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
Expand Down
103 changes: 80 additions & 23 deletions tests/flow/test_call_subquery.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,30 +41,87 @@ 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",
Copy link
Collaborator

Choose a reason for hiding this comment

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

what's the issue with this last query ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Non-simple import in the second branch of the UNION.

]
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
""",
]
for query, err in queries_errors:
self.expect_error(query, err)
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: 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 = [
Expand Down