-
Notifications
You must be signed in to change notification settings - Fork 3
graphql: implement c-style expressions parser #209
graphql: implement c-style expressions parser #209
Conversation
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.
Thanks for the patch!
I think it has quite good quality already, but left thoughts on possible improvements (some of them are just for consistency with the rest code base) and rice some questions. BTW, questions from a reviewer often are good candidates to describe things in a comment or a commit message.
Use branch inside the repository instead of a fork: it eases review of the changes locally (just git checkout).
It is good practice to add the prefix 'WIP: ' for pull requests in work to avoid misunderstanding of the status and prevent premature check-in.
Please, consider comments below. Consider they as recommendations that are debatable. Consider a test comments with lower priority then ones of the code.
4beadc9
to
271c510
Compare
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.
I look briefly into the executor. Please, consider comments below. The most important one is API proposal.
Why do you say here that the commit implements expression parser? It is not so, this commit implements only executor.
prev = prev or second_operand | ||
end | ||
|
||
-- Equal. |
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.
TBD: think how to handle nil / box.NULL with equal and other operation. Like SQL does, ternary logic?
@Hollow111 BTW, how to break a line within an expression? |
If i get you right: just use multiline lua-string and break lines as usual (it behaves the same as space). e.g.:
|
@Hollow111 Thanks! It seems that our graphql parser is not ready for this. I have filed the issue about is: #223. It should not be part of this PR, though. |
271c510
to
0895398
Compare
Implement c-style filter expressions for graphql objects. Introduced executor into expressions module. Moved regexp implementation into utils.lua. Needed for tarantool#13
0895398
to
ecdb29d
Compare
Needed for #13