zhztheplayer commented on a change in pull request #1041: [CALCITE-2847]
Optimize global LOOKAHEAD for SQL parsers
URL: https://github.com/apache/calcite/pull/1041#discussion_r264518804
##########
File path: core/src/main/codegen/templates/Parser.jj
##########
@@ -1698,94 +1700,97 @@ SqlNode FromClause() :
{
e = TableRef()
(
- // Decide whether to read a JOIN clause or a comma, or to quit having
- // seen a single entry FROM clause like 'FROM emps'. See comments
- // elsewhere regarding <COMMA> lookahead.
LOOKAHEAD(2)
- natural = Natural()
- joinType = JoinType()
- e2 = TableRef()
(
- <ON> {
- joinConditionType = JoinConditionType.ON.symbol(getPos());
- }
- condition = Expression(ExprContext.ACCEPT_SUB_QUERY) {
+ // Decide whether to read a JOIN clause or a comma, or to quit
having
+ // seen a single entry FROM clause like 'FROM emps'. See comments
+ // elsewhere regarding <COMMA> lookahead.
+ LOOKAHEAD(3)
Review comment:
Update:
Sadly, I found that we still need this LOOKAHEAD(3) here. Making it 2 fails
some test cases, for instance, [this
one](https://github.com/apache/calcite/blob/master/core/src/test/java/org/apache/calcite/sql/parser/SqlParserTest.java#L2390).
I have added some comments to code to explain the reason:
https://github.com/apache/calcite/blob/6c4a4d101fab708f54518f7377b7f21a469f5796/core/src/main/codegen/templates/Parser.jj#L1710-L1719
And I'll take time to find the reason why the test cases could pass before
this PR. For now I think it's good to make it just LOOKAHEAD(3).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
