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
Refactor rewrite star projection #3136
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #3136 +/- ##
==========================================
+ Coverage 90.85% 90.86% +0.01%
==========================================
Files 294 294
Lines 30269 30304 +35
==========================================
+ Hits 27502 27537 +35
Misses 2767 2767
☔ View full report in Codecov by Sentry. |
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.
Nice work, see some comments.
@@ -238,7 +217,6 @@ static bool _rewrite_call_subquery_star_projections | |||
|
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.
No need for casting here (can also remove the new-line).
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. Casting removed
# assert results | ||
self.env.assertEquals(len(res.result_set), 1) | ||
self.env.assertEquals(res.result_set[0], [1, 2]) | ||
|
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.
The following case fails:
GRAPH.QUERY g "WITH 1 AS one CALL {RETURN one AS num UNION WITH * WITH * WITH one AS num} RETURN num"
I think the separation between the identifiers
and the local_identifiers
in _rewrite_call_subquery_star_projections()
is probably necessary, but it needs some work.
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. New tests were added. Now we don't collect identifiers for the RETURN
clauses in _rewrite_call_subquery_star_projections()
, the identifiers are collected later.
if(t == CYPHER_AST_WITH) { | ||
collect_with_projections(clause, local_identifiers); | ||
} else { | ||
collect_return_projections(clause, local_identifiers); | ||
} |
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.
We enter these collection functions for a second time if we have a *
(first in 252, 254).
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. Please check the changes done to the function _rewrite_call_subquery_star_projections()
, I think now it is simpler.
Refactor
AST_RewriteStarProjections()
and_rewrite_call_subquery_star_projections()