Call subquery validate return #3107

nafraf · 2023-06-21T18:53:11Z

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

codecov · 2023-06-21T18:53:35Z

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.01 ⚠️

Comparison is base (9c360b2) 90.87% compared to head (aae0e80) 90.87%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3107      +/-   ##
==========================================
- Coverage   90.87%   90.87%   -0.01%     
==========================================
  Files         294      294              
  Lines       30269    30257      -12     
==========================================
- Hits        27508    27495      -13     
- Misses       2761     2762       +1

Impacted Files	Coverage Δ
src/ast/ast.c	`92.16% <100.00%> (+0.24%)`	⬆️
src/ast/ast_validations.c	`97.46% <100.00%> (+0.34%)`	⬆️

... and 4 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

raz-mon

Nice work!
See few comments.

src/ast/ast.c

src/ast/ast_validations.c

src/ast/ast.c

src/ast/ast.h

swilly22 · 2023-07-03T07:21:31Z

tests/flow/test_call_subquery.py

+            "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",


what's the issue with this last query ?

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

tests/flow/test_call_subquery.py

src/ast/ast.h

swilly22 · 2023-07-03T07:30:56Z

src/ast/ast.c

@@ -580,6 +580,26 @@ bool AST_ClauseContainsAggregation
 	return aggregated;
 }

+const char *AST_GetProjectionAlias


Please copy the comments from the header file to this file.

OK. Done in 23fa13d

swilly22 · 2023-07-03T07:32:37Z

src/ast/ast.c

+		const char *alias = AST_GetProjectionAlias(projection);
+		if(alias != NULL) {
+			array_append(columns, alias);
+		}


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

OK. Condition removed in 23fa13d

swilly22 · 2023-07-03T07:33:57Z

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);


is it possible for AST_GetProjectionAlias to return NULL ?

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

swilly22 · 2023-07-03T07:35:08Z

src/ast/ast_validations.c

@@ -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 =


are we using alias_node ?

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

swilly22 · 2023-07-03T07:35:42Z

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);


In which case AST_GetProjectionAlias returns NULL ?

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

OK. The condition was removed.

swilly22 · 2023-07-03T07:36:08Z

src/ast/ast.h

+// 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


Couldn't find AST_GetSubqueryProjectionAlias implementation.

We don't need this function. Its definition was deleted in 23fa13d. Thanks

nafraf added 2 commits June 21, 2023 11:24

Validate CALL{ RETURN literal}

428d42c

Update deps/libcypher-parser

0daca85

nafraf and others added 2 commits June 21, 2023 13:55

Merge branch 'master' into call_subquery_validate_return

d950f8a

Continue if alias is null

e084f48

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

raz-mon reviewed Jun 22, 2023

View reviewed changes

src/ast/ast.c Outdated Show resolved Hide resolved

src/ast/ast.c Outdated Show resolved Hide resolved

src/ast/ast_validations.c Outdated Show resolved Hide resolved

src/ast/ast_validations.c Outdated Show resolved Hide resolved

src/ast/ast_validations.c Outdated Show resolved Hide resolved

nafraf and others added 5 commits June 22, 2023 08:29

Add function AST_GetProjectionAlias()

c3d596c

Merge branch 'master' into call_subquery_validate_return

6deb8e2

AST_GetProjectionAlias in _Validate_call_subquery

f56d1dc

WIP: Print time taken in test_AbortRunningTask()

0eb28d5

Revert changes done in test_cron.c

ada954b

raz-mon requested changes Jun 25, 2023

View reviewed changes

src/ast/ast.c Outdated Show resolved Hide resolved

src/ast/ast.h Outdated Show resolved Hide resolved

nafraf and others added 9 commits June 25, 2023 12:42

Comments corrections

87e4298

Merge branch 'master' into call_subquery_validate_return

63ffebe

Merge branch 'master' into call_subquery_validate_return

4b3190c

Merge branch 'master' into call_subquery_validate_return

eb3be08

Merge branch 'master' into call_subquery_validate_return

052e11a

Merge branch 'master' into call_subquery_validate_return

4487073

Add function AST_GetSubqueryProjectionAlias()

6e76e74

Merge branch 'master' into call_subquery_validate_return

ac8ce42

Use cypher_ast_projection_is_aliased()

44bfc19

nafraf mentioned this pull request Jun 29, 2023

Add cypher_ast_projection_is_aliased() RedisGraph/libcypher-parser#27

Open

swilly22 requested changes Jul 3, 2023

View reviewed changes

nafraf and others added 4 commits July 3, 2023 08:01

Remove unneeded validations

23fa13d

Use AST_GetProjectionAlias() in AST_BuildCallColumnNames()

5c3e536

Revert removing include

8361b4a

Merge branch 'master' into call_subquery_validate_return

aae0e80

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Call subquery validate return #3107

Call subquery validate return #3107

nafraf commented Jun 21, 2023

codecov bot commented Jun 21, 2023 •

edited

raz-mon left a comment

swilly22 Jul 3, 2023

raz-mon Jul 3, 2023

swilly22 Jul 3, 2023

nafraf Jul 3, 2023

swilly22 Jul 3, 2023

nafraf Jul 3, 2023

swilly22 Jul 3, 2023

nafraf Jul 3, 2023 •

edited

swilly22 Jul 3, 2023

nafraf Jul 3, 2023

swilly22 Jul 3, 2023

raz-mon Jul 3, 2023

nafraf Jul 3, 2023

swilly22 Jul 3, 2023

nafraf Jul 3, 2023

Call subquery validate return #3107

Are you sure you want to change the base?

Call subquery validate return #3107

Conversation

nafraf commented Jun 21, 2023

codecov bot commented Jun 21, 2023 • edited

Codecov Report

raz-mon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nafraf Jul 3, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 21, 2023 •

edited

nafraf Jul 3, 2023 •

edited