-
Notifications
You must be signed in to change notification settings - Fork 59
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
feat: add binary between expression #117
Conversation
grammar.js
Outdated
prec.left(precendence, seq( | ||
field('left', $.identifier), | ||
field('operator', operator), | ||
field('low', $.literal), | ||
$.keyword_and, | ||
field('high', $.literal) | ||
)) |
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'm not sure if this answers your question, I can look into it more later but from looking at this I don't think between
& not between
should be binary_expression
nodes. They might deserve their own nodes.
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 have made it an extra node. However, as soon as the not
token in in there it fails because it detects the where clause as a binary_expression
I also changes _high
and low
to _expression
since one could even put a subquery in there
Looks like select a from b where a not in (select 1) has the same issue and is not working correctly. |
@DerekStride did you had time to take a look? I am bit stuck here. |
I haven't had a chance yet but I'll take a look later today |
I gave it a try and found this solution, it's not great but I think it'll work for now: e9cf694 |
grammar.js
Outdated
[seq($.keyword_not, $.keyword_between), 'not_between'], | ||
].map(([operator, precedence]) => | ||
prec.left(precedence, seq( | ||
field('left', $.identifier), |
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.
Another option, is to use field here and then try to detangle the precedences, but I couldn't get that to work:
- field('left', $.identifier),
+ field('left', $.field),
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, I was in that loop of trying to untangle the precedence already. I was not able to get this to work. I would suggest that we go with the $._expression
for now. If I find motivation I will try to refactor the $_expression
logic. For now it will be more permissive that we are probably aiming for.
Let me rebase and push it to update the PR
2fb8be4
to
cb43e35
Compare
feat: between as extra node feat: (not) between expressions
cb43e35
to
a8622bb
Compare
src/parser.c
Outdated
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.
With #100 we don't need the src/*
files checked into main anymore so they can be removed from the PR
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.
Just the 1 change to remove the parser artifacts and this is good to go 👍
This PR adds
between
expressions for where clauses likeselect a from b where a between '2022-01-01' and '2022-02-01'
or
select a from b where a not between 1 and 2
@DerekStride
not between
is currently not working. Do you have any clue why? It is structured exactly the same asnot like
but it still gives an error.