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

Support parameter in LIMIT clause #4529

Merged
merged 2 commits into from
Jul 24, 2020
Merged

Conversation

kasiafi
Copy link
Member

@kasiafi kasiafi commented Jul 21, 2020

No description provided.

@cla-bot cla-bot bot added the cla-signed label Jul 21, 2020
@martint martint self-requested a review July 21, 2020 21:11
Copy link
Member

@martint martint left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple of minor comments and a question.

analysis.getParameters()));
}
catch (VerifyException e) {
throw semanticException(TYPE_MISMATCH, node, "Non constant parameter value for LIMIT: %s", providedValue);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TYPE_MISMATCH is not an appropriate code for this error. Let's go with INVALID_ARGUMENTS

@Test
public void testLimitInvalidRowCount()
{
assertFails("SELECT * FROM t1 LIMIT 987654321098765432109876543210")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happened to this case?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't throw PrestoException any more.
The row count used to be fetched as text and analysed in the StatementAnalyzer. After this change, a LongLiteral is instantiated in AstBuilder, and ParsingException is thrown.
Similarly, negative values now fail in Parser.

@@ -164,8 +164,14 @@ queryNoWith:
queryTerm
(ORDER BY sortItem (',' sortItem)*)?
(OFFSET offset=INTEGER_VALUE (ROW | ROWS)?)?
((LIMIT limit=(INTEGER_VALUE | ALL)) | (FETCH (FIRST | NEXT) (fetchFirst=INTEGER_VALUE)? (ROW | ROWS) (ONLY | WITH TIES)))?
;
((LIMIT limitRowCount) | (FETCH (FIRST | NEXT) (fetchFirst=INTEGER_VALUE)? (ROW | ROWS) (ONLY | WITH TIES)))?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should do the same for FETCH FIRST and OFFSET, but this can be addressed in a separate PR

@kasiafi kasiafi force-pushed the 100LimitParameter branch 3 times, most recently from 2605ecb to 73131a8 Compare July 24, 2020 10:06
@kasiafi kasiafi force-pushed the 100LimitParameter branch from 73131a8 to 6f0283d Compare July 24, 2020 11:57
@martint martint merged commit 07ab946 into trinodb:master Jul 24, 2020
@martint martint added this to the 340 milestone Jul 24, 2020
@martint martint mentioned this pull request Jul 25, 2020
8 tasks
@mosabua mosabua added the needs-docs This pull request requires changes to the documentation label Aug 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed needs-docs This pull request requires changes to the documentation
Development

Successfully merging this pull request may close these issues.

3 participants