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
Remove grammar duplication #62
Remove grammar duplication #62
Conversation
Any feedback on this pull request ? |
Hi sorry I didn't see this sooner! Something must have failed in the notification system. IIRC the reason that part of the grammar is duplicated is so that there are no shift reduce conflicts in the BETWEEN section. It's possible that this is no longer necessary. |
@ricomariani has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
It looks like it should work but an internal test is failing. I'll look into that. Your way is cleaner than my way and still avoids the conflict. |
Minimal repro... select CAST(1 AS REAL) + 1; No longer parses investigating... |
Yeah this doesn't work... sigh. The point of To do this Compare:
This allows the LHS of + to be any
This limits + operations to
In particular, logicals are not allowed in math_expr so |
I'll add a test case to force the issue. |
The new test cases should be landing shortly. Any objection if I close the PR at this point? If you can think of a way to remove that redundancy I'd be most grateful. I don't like it :D |
@@ -848,17 +848,7 @@ math_expr[result]: | |||
; | |||
|
|||
expr[result]: | |||
basic_expr { $result = $basic_expr; } | |||
| expr[lhs] '&' expr[rhs] { $result = new_ast_bin_and($lhs, $rhs); } |
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.
Yeah the problem here is that if you use math_expr for all of these then the LHS and RHS are limited to math_expr which means you cast parse something like CAST(1 as REAL) + 1
I'm adding new test cases to give a clean error on that.
The above are some new simple test cases that cover this case. I'm sorry those should have been there in the first place -- then this would have been reasonably obvious. Thanks for helping me find that gap. |
Thank you for all feedback ! |
Worth reading this comment in PostgreSQL grammar https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/backend/parser/gram.y;h=3adc087e3ffe59a5c4bf5443d7e49545f5f0ba51;hb=refs/heads/REL_13_STABLE#l13149 |
This is a good place for a macro preprocessor to have been incorporated in bison/yacc. |
For my own I normally use a simple in place custom preprocessor for mechanical duplication/generation (it was inspired by one in python but I don't remember it's origin right now):
Here is the one I use most:
The same in Lua (a bit more verbose):
|
Actually we can totally fix this! We just didn't go far enough. We need to move almost everything into math_expr so that only the things that ware weird with BETWEEN are out. We're left with this and no duplication. I do have to add a NOT BETWEEN token but that's not so bad.
This also reveals a couple of lingering errors... |
Well poop. This almost works. If you pull everything into math_expr that isn't AND/OR/BETWEEN you're golden. The only problem is that if you put NOT into math_expr then it forces the order of operations of NOT to be wrong... NOT has to be weaker than BETWEEN. And btw this forced me to look hard at order of operations so I had to add more tests because there were bugs... On the other hand if you put NOT in expr and not in math expr then you have the problem that you can't parse
If NOT was tighter than between this would work nicely... but it isn't. Meaning
Sigh... |
I was able to make it work! I had to also move the between node into math_expr. So the main expr node contains only AND/OR/COLLATE that break works and requires no duplication. All we needed from math expr was that it not have AND in it. I had to do a little tweak to make BETWEEN left associative as expected but that was easy. |
This PR turned out to be very good at finding latent bugs... thank you! |
@ricomariani merged this pull request in 3610bed. |
I generalized what you did:
This works well and has no duplication. |
FWIW it seems like PostgreSQL had settled on a solution very much like the one I had, my basic_expr, math_expr, and expr correspond to their a_ b_ and c_ exprs (not in that order). Interestingly, the order of operations split working out the way that it does let's you cleave out AND/OR and create a chain. I think they could do the same in their grammar if they were so inclined. |
If you look at 9185fc8 you will see all the bugs this forced out. I did those fixes in a separate diff because they were unrelated to your refactor, but it was your PR that led me to find those bugs. |
Two people looking at the same thing not always see the same thing ! |
Looking at the sqlite3 railroad diagram (https://sqlite.org/forum/forumpost/c7a0c2a23231a27f9b746f99e390e1a89d83a4678eda306b4ae0415c471aa819) I can see that sqlite grammar doesn't have two kinds of expressions and I'm wondering if the bugs you've found in |
Testing this query in both sqlite3 and PostgreSQL gives:
PostgreSQL:
|
This query runs on both sqlite3 and PostgreSQL and give different results, it seems that we have a bug on PostgreSQL:
Postgresql at (https://extendsclass.com/postgresql-online.html):
|
Here is the bug report on PostgreSQL just in case you want to follow it:
|
It seems that I fired the trigger to early, although the output of that website does seems wrong when executing the query on other places with different PostgreSQL version the output is the expected one:
|
It seems that there is an unnecessary grammar duplication that this pull request removes.