Call subquery validate return by nafraf · Pull Request #3107 · RedisGraph/RedisGraph · GitHub
Skip to content

Call subquery validate return - #3107

Open
nafraf wants to merge 22 commits into
masterfrom
call_subquery_validate_return
Open

nafraf wants to merge 22 commits into
masterfrom
call_subquery_validate_return

Conversation

@nafraf

@nafraf nafraf commented Jun 21, 2023

Copy link
Copy Markdown
Contributor

Patch to validate that the returning literals in CALL subquery are aliased.

@codecov

codecov Bot commented Jun 21, 2023 •

Copy link
Copy Markdown

@raz-mon
raz-mon self-requested a review June 22, 2023 05:56

@raz-mon raz-mon left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nice work!
See few comments.

Comment thread src/ast/ast.c Outdated
Comment thread src/ast/ast.c Outdated
Comment thread src/ast/ast_validations.c Outdated
Comment thread src/ast/ast_validations.c Outdated
Comment thread src/ast/ast_validations.c Outdated
Comment thread src/ast/ast.c Outdated
Comment thread src/ast/ast.h Outdated
"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
Copy Markdown
Contributor

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
Copy Markdown
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.

Comment thread tests/flow/test_call_subquery.py
Comment thread src/ast/ast.h Outdated
Comment thread src/ast/ast.c
return aggregated;
}

const char *AST_GetProjectionAlias

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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

Comment thread src/ast/ast.c Outdated
Comment on lines +616 to +619
const char *alias = AST_GetProjectionAlias(projection);
if(alias != NULL) {
array_append(columns, alias);
}

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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

Comment thread src/ast/ast_validations.c
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
Copy Markdown
Contributor

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 ?

@nafraf nafraf Jul 3, 2023 •

Copy link
Copy Markdown
Contributor Author

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 || ...

Comment thread src/ast/ast_validations.c Outdated
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
Copy Markdown
Contributor

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
Copy Markdown
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.

Comment thread src/ast/ast_validations.c
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
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
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.

Comment thread src/ast/ast.h Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Couldn't find AST_GetSubqueryProjectionAlias implementation.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants