cleishm / libcypher-parser

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

added params type support #25

Closed DvirDukhan closed 4 years ago

DvirDukhan commented 4 years ago

Hi @cleishm This PR adds the ability to know the types for given parameters. The use case for this feature is when given a parameterized query and its parameters, the libcypher-parser users can get the type of the parameters, and as a direct result, to evaluate the value of any given query parameter.

cleishm commented 4 years ago

Hi @DvirDukhan!

Wow, it's great to see this contribution - including tests :)

However, I'm not sure I understand why this change is needed, and I'm not sure we can apply it. Specifically, because it would now reject valid queries - e.g. CYPHER runtime=compiled ....

The Cypher query options are not literals like in the statements of the query, and string values in particular do not need to be quoted like they do elsewhere: https://neo4j.com/docs/cypher-manual/current/query-tuning/query-options/.

If we changed this, then all the valid queries that don't quote query parameters would start being rejected.

Thoughts?

Cheers, Chris

DvirDukhan commented 4 years ago

Hi @cleishm Thanks for pointing this usage of cypher parameters, I was not aware of that, and the current change seems to be misplaced and misuse of this section of the query. However, I have noticed that libsypher-parser does have the ability to parse the cypher/Neo4J client-side commands, which start with a colon (:). Given that, the current command to set a parameter value is :param x=>1. (You can type :help param at Neo4J web app console for additional information) The current version of licypher-parser does not support this specific syntax for parameter value set, for example, the command above yields a parsing error, however, the output of libcypher-parser for the command :param x=1 is

@0  0..10  command   name=@1, args=[@2]
@1  1..6   > string  "param"
@2  7..10  > string  "x=1"

I suggest that given this specific syntax for parameters settings commands I will:

  1. revert the changes to the cypher parameters.
  2. add grammar rules to
  3. implement grammar rules for parameter setting command, that will yield the parameter type and value, in the linter output.

If you have any input regarding setting parameters value in a query or command, it will be highly valued.

I'm waiting for your feedback. Thanks!

cleishm commented 4 years ago

So the client commands (:<name>) are not defined in the Cypher specification - they vary by client. However, the parser does handle the input :param x=>1, as follows:

% echo ':param x=>1' | cypher-lint -a
@0  0..11  command   name=@1, args=[@2]
@1  1..6   > string  "param"
@2  7..11  > string  "x=>1"

and also

% echo ':param x => 1' | cypher-lint -a
@0   0..13  command   name=@1, args=[@2, @3, @4]
@1   1..6   > string  "param"
@2   7..8   > string  "x"
@3   9..11  > string  "=>"
@4  12..13  > string  "1"

It's handling this, because it simply determines the command and then tokenizes arguments based on whitespace (pretty much the same parsing rules as sh uses). The intention is that the client can then interpret those however it likes.

However, I can see some limitations to this, so happy to revisit it. I'm wondering if the right approach is to parse it into a command name and a single argument (minus leading and trailing whitespace). The user of the library would then be responsible for the tokenization, based on whatever that command means to them. And to make it easier to interpret literals, we could provide an API in the library for parsing literals specifically. WDYT?

DvirDukhan commented 4 years ago

@cleishm thanks for the clarification. I probably used a wrong syntax where the parser failed, sorry for that. Regarding further implementation: At RedisGraph, we are in the phase that we want to implement the parameterized queries feature. As you know, we are using your libcypher-parser as our parser, which allows us to achieve significant progress regarding our cypher coverage. The documentation regarding the syntax of how to write parameterized queries, with parameters set, is severely lacking both in Neo4J and openCypher resources. Given that, we are trying to understand what is the best way to do it, facilitating libcypher-parser. After consulting with my colleagues, unfortunately, I understand that every proposal that I will offer is as good as a guess. I have decided to hold further implementation proposals for libcypher-parser, regarding parameters sending until I verify the syntax formally with Neo4J/openCypher. I will update you regarding their answers so we can continue the development of this PR.

cleishm commented 4 years ago

I'd be interested in what Neoj4/openCypher has to say on this, but I think the whole point in the client commands is that they are supposed to be flexible and vary for each implementation, as each implementation has different capabilities. That's why we originally introduced those client commands, rather than building it into the Cypher syntax itself.

Hence I'm suggesting we open up a literal parsing API, so we can expose the parts that are consistent between all implementations - and let the implementation easily construct the client command set that makes sense in their usage of Cypher.

cleishm commented 4 years ago

If this is still valid, please feel free to reopen against the main branch.