cleishm / libcypher-parser

Cypher Parser Library
Apache License 2.0
147 stars 39 forks source link

Path patterns support #26

Closed gsvgit closed 4 years ago

gsvgit commented 4 years ago

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 commented 4 years ago

Wow. This is amazing.

I'll take a look through!

cleishm commented 4 years ago

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 commented 4 years ago

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.

gsvgit commented 4 years ago

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

cleishm commented 4 years ago

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