Skip to content
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

Minor cleanup in parsing error handler #1042

Merged
merged 4 commits into from Jun 27, 2019

Conversation

2 participants
@findepi
Copy link
Member

commented Jun 25, 2019

No description provided.

@findepi findepi requested a review from martint Jun 25, 2019

@cla-bot cla-bot bot added the cla-signed label Jun 25, 2019

@@ -182,6 +182,12 @@ public Analyzer(
int tokenIndex = current.tokenIndex;
CallerContext caller = current.caller;

while (tokenIndex < stream.size() && stream.get(tokenIndex).getType() == SqlBaseParser.WS) {

This comment has been minimized.

Copy link
@findepi

findepi Jun 25, 2019

Author Member
  • tokenIndex < stream.size() is redundant
  • SqlBaseParser.WS should be provided as configuration

This comment has been minimized.

Copy link
@martint

martint Jun 26, 2019

Member

Do this instead: stream.get(tokenIndex).getChannel() == Token.HIDDEN_CHANNEL

Show resolved Hide resolved presto-parser/src/main/java/io/prestosql/sql/parser/ErrorHandler.java Outdated

@findepi findepi force-pushed the findepi:error-handler branch 2 times, most recently from cb6af51 to d6bc2ca Jun 26, 2019

@martint
Copy link
Member

left a comment

Looks good, but check the potential out-of-bounds condition.

@@ -182,6 +182,12 @@ public Analyzer(
int tokenIndex = current.tokenIndex;
CallerContext caller = current.caller;

while (stream.get(tokenIndex).getChannel() == Token.HIDDEN_CHANNEL) {

This comment has been minimized.

Copy link
@martint

martint Jun 27, 2019

Member

I think this will fail if the trailing tokens are all whitespace -- it will run out of bounds

This comment has been minimized.

Copy link
@findepi

findepi Jun 27, 2019

Author Member

I think EOF is always the last token and it's DEFAULT_CHANNEL (not HIDDEN_CHANNEL), so I don't need explicit bound check.
(Previously the code depended on this too, bounds were never checked explicitly.)

This comment has been minimized.

Copy link
@findepi

findepi Jun 27, 2019

Author Member

I added one more test case: "CREATE TABLE foo " (the token stream ends with ...WS, EOF).

btw ANTLR emits only one WS token for all consecutive whitespace, so technically the while loop is not necessary. I am keeping it to avoid reader's astonishment.

@findepi findepi force-pushed the findepi:error-handler branch 4 times, most recently from 7e470d1 to e450457 Jun 27, 2019

findepi added some commits Jun 27, 2019

Add more parser error handling tests
And remove one duplicate test case.
Expand special rules in ErrorHandler
When parsing fails in some ambiguous place, the `ErrorHandler` is
invoked at last certain position, which might be `expression` or
`primaryExpression`. By expanding special rules, we may produce
more accurate suggestions.
Remove erroneous comment
Window frame bound may be a non-literal expression. This is already
covered by existing test case
(`io.prestosql.tests.AbstractTestWindowQueries#testWindowFrames`).

@findepi findepi force-pushed the findepi:error-handler branch from e450457 to 6b29fbf Jun 27, 2019

@findepi findepi merged commit 304e8b3 into prestosql:master Jun 27, 2019

2 checks passed

Travis CI - Pull Request Build Passed
Details
verification/cla-signed
Details

@findepi findepi deleted the findepi:error-handler branch Jun 27, 2019

@findepi findepi referenced this pull request Jun 27, 2019

Closed

Release notes for 316 #1000

5 of 6 tasks complete

@findepi findepi added this to the 316 milestone Jun 27, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.