-
Notifications
You must be signed in to change notification settings - Fork 7k
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
CLICKHOUSE-3211 add TOP m, and OFFSET k #2840
CLICKHOUSE-3211 add TOP m, and OFFSET k #2840
Conversation
What does this do? It looks like it's the same as the |
@zhang2014 You are right) It seems like a |
6751752
to
6e46121
Compare
So why do we need it? I'm just curious : ) |
@zhang2014 |
Does this solve this issue in #2198? |
LIMIT OFFSET is required for compatibility. And |
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.
We should throw an exception if offset is specified in both forms LIMIT 10, 20 OFFSET 30
or if both LIMIT
and TOP
are specified.
Test should check that exceptions are thrown.
INSERT INTO test.test VALUES (8); | ||
INSERT INTO test.test VALUES (9); | ||
|
||
SELECT TOP 2 * FROM test.test; |
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.
The test is non-deterministic due to lack of ORDER BY.
SELECT TOP 2 * FROM test.test; | ||
SELECT TOP (2) * FROM test.test; | ||
SELECT * FROM test.test LIMIT 2 OFFSET 2; | ||
SELECT TOP 2 * FROM test.test LIMIT 2; -- { clientError 406 } |
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.
Negative test for LIMIT n, m OFFSET k
is pending.
I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en