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

Path patterns support #26

Closed
wants to merge 22 commits into from

Conversation

gsvgit
Copy link

@gsvgit gsvgit commented Apr 17, 2020

Hi @cleishm

The syntax for path patterns as described in this proposal (with this fix) is supported. This extension allows one to express RPQs and CFPQs (regular and context-free path queries) in Cypher.

@cleishm
Copy link
Owner

cleishm commented Apr 17, 2020

Wow. This is amazing.

I'll take a look through!

@cleishm
Copy link
Owner

cleishm commented Apr 18, 2020

Quick question while I'm reviewing. I'm unfamiliar with the latest CIP, so perhaps I'm not following. From the CIP, it seems that "path base" can never appear as an AST node itself:

PathBase = PathEdge
         | PathAny
         | PathNode
         | PathReference
         | PathGroup
         ;

But this PR does add it as a concrete AST node. If there is common functionality or the need for a common base type (likely), then perhaps it could be a sub-type instead? Examples of this in the current codebase are cypher_expression_astnode_vt and cypher_query_option_astnode_vt.

@simpletonDL
Copy link

Hi, Chris
Thanks for the detailed question. This is probably not a very suitable name for this concrete AST node. In my mind PathBase is union of PathRepetition and PathDirection, so it stores direction and range details. Something like this:

PathBase = ['<'], SomeNode, ['>'], [('*', [RangeDetail]) | '+' | '?'];
SomeNode = PathEdge
         | PathAny
         | PathNode
         | PathReference
         | PathGroup
         ;

I maybe don`t understand what exactly sub-type is, but it doesn't look like a something that can contains useful fields. So I believe that this is not what we want.

named-path-clause =
< PATHPATTERN i:identifier EQUAL - p:pattern-path -
(WHERE e:expression | e:_null_)
> { $$ = named_path_predicate(i, p, e); }

Choose a reason for hiding this comment

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

Here we use named_path_predicate (almost the same as named_path) for named path patterns construction. So the node of type CYPHER_AST_NAMED_PATH may appear both as child of pattern and as child of query. Named path patterns and named paths are structurally identical but different in semantics. So should we create new concrete AST node for named-path-clause in this case?

@gsvgit
Copy link
Author

gsvgit commented Jun 18, 2020

Hi @cleishm!
For what reason this pull request was closed?

@cleishm
Copy link
Owner

cleishm commented Jun 18, 2020

@gsvgit Ah - not intentional. Please feel free to re-open against the main branch!

@gsvgit gsvgit mentioned this pull request Jul 2, 2020
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.

None yet

4 participants