[jira] [Comment Edited] (CALCITE-2847) Optimize global LOOKAHEAD for SQL parsers

["Hongze Zhang (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/CALCITE-2847?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16809449#comment-16809449 ] 

Hongze Zhang edited comment on CALCITE-2847 at 4/4/19 3:30 AM:
---------------------------------------------------------------

Thanks for the review, [~julianhyde] and [~krisden]! But there is a tiny issue[1] founded in the PR (thanks, [~zhenw]!), and I'll summarize the problem by 2 phases:

# The parser is generated with correct lookahead code for `<CROSS> <JOIN>` under global settings `LOOKAHEAD=2`;
# The parser is generated with incorrect lookahead code for `<CROSS> <JOIN>` under global settings `LOOKAHEAD=1` and local lookahead hint `LOOKAHEAD(2) Natural() JoinType()`.

After a little debug and investigation I see that JavaCC generates minimum lookahead code[2] incorrectly for the expansion `<CROSS> <JOIN>` when we use the global lookahead 1 and lookahead hint 2 for expansion `Natural() JoinType()` (phase 2). Where we didn't meet the issue under the phase 1 because JavaCC assigns lookahead 2 everywhere including the expansion `<CROSS> <JOIN>`.

For solving the "incorrect minimum lookahead code" problem, I've opened a PR[3] for JavaCC project. And currently I am about to push this forward (maybe merge after next 24 hours) by treating the LOOKAHEAD(3) as a necessary workaround, since we already have a "+1" in previous discussion. Also, any further review will be much appreciated.

Besides, [~julianhyde], could you please check if any other things need to be done? I can hold off at anytime.

[1] [https://github.com/apache/calcite/pull/1041#discussion_r264518804]
[2] [https://github.com/zhztheplayer/javacc-calcite-generated-sources/blob/with-7.0.4/SqlParserImpl.java#L24929-L24933]
[3] [https://github.com/javacc/javacc/pull/87]


was (Author: zhztheplayer):
Thanks for the review, [~julianhyde] and [~krisden]! But there is a tiny issue[1] founded in the PR (thanks, [~zhenw]!), and I'll summarize the problem by 2 phases:

# The parser is generated with correct lookahead code for `<CROSS> <JOIN>` under global settings `LOOKAHEAD=2`;
# The parser is generated with incorrect lookahead code for `<CROSS> <JOIN>` under global settings `LOOKAHEAD=1` and local lookahead hint `LOOKAHEAD(2) Natural() JoinType()`.

After a little debug and investigation I see that JavaCC generates minimum lookahead code[2] incorrectly for the expansion `<CROSS> <JOIN>` when we use the global lookahead 1 and lookahead hint 2 for expansion `Natural() JoinType()` (phase 2). Where we didn't meet the issue under the phase 1 because JavaCC assigns lookahead 2 everywhere including the expansion `<CROSS> <JOIN>`.

For solving the "incorrect minimum lookahead code" problem, I've opened a PR[3] for JavaCC project. And currently I am about to push this forward (maybe merge after next 24 hours) by treating the LOOKAHEAD(3) as a necessary workaround, since we already have a "+1" in previous disscussion. Also, any further review will be much appreciated too.

[1] [https://github.com/apache/calcite/pull/1041#discussion_r264518804]
[2] [https://github.com/zhztheplayer/javacc-calcite-generated-sources/blob/with-7.0.4/SqlParserImpl.java#L24929-L24933]
[3] [https://github.com/javacc/javacc/pull/87]

> Optimize global LOOKAHEAD for SQL parsers
> -----------------------------------------
>
>                 Key: CALCITE-2847
>                 URL: https://issues.apache.org/jira/browse/CALCITE-2847
>             Project: Calcite
>          Issue Type: Improvement
>          Components: core
>            Reporter: Hongze Zhang
>            Assignee: Hongze Zhang
>            Priority: Major
>              Labels: pull-request-available
>             Fix For: 1.20.0
>
>          Time Spent: 1.5h
>  Remaining Estimate: 0h
>
> Currently global LOOKAHEAD is set to 2 for the built-in SQL parsers[1], we'd like to optimize LOOKAHEAD to 1 to enable performance enhancement if possible.
> [1]https://github.com/apache/calcite/blob/883666929478aabe07ee5b9e572c43a6f1a703e2/core/pom.xml#L304



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)