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

add arbitrary const expressions in limit processing #4246

Merged

Conversation

k3box
Copy link

@k3box k3box commented Feb 3, 2019

I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en

For changelog. Remove if this is non-significant change.

Category (leave one):

  • New Feature

Short description (up to few sentences):
Support for arbitrary constant expressions in LIMIT clause.

@k3box
Copy link
Author

k3box commented Feb 3, 2019

Здравствуйте, я вроде как написал код по задаче "Произвольные константные выражения в LIMIT", но не могу его протестировать :/

  1. Нужно запустить сервер - он у меня запускается, но с какими-то подозрительными сообщениями: http://bit.ly/2UEHrCk
  2. Нужно скопировать клиент в /usr/bin, но на маке, кажется, нельзя сделать это без специальных ухищрений а-ля запуск ОС в диагностическом режиме. Наверняка есть какой-то workaround на эту тему =)
  3. Соответственно, тесты падают уже на 9-м (http://bit.ly/2UCwIbC), причём опция -k (продолжать, пока не сфейлится N тестов) почему-то не действует...

Помогите, пожалуйста =)
Если что, мой яндексовый ник - karnienko@, так что могу дойти ногами :)

}
}


void InterpreterSelectQuery::getLimitUIntValue(const ASTPtr& ptr, size_t& result)
Copy link
Member

Choose a reason for hiding this comment

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

Why not return by value?

if (!isNumber(eval_result.second)) {
throw Exception("Illegal limit expression", ErrorCodes::INVALID_LIMIT_EXPRESSION);
}
result = applyVisitor(FieldVisitorConvertToNumber<UInt64>(), eval_result.first);
Copy link
Member

Choose a reason for hiding this comment

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

Overflow may happen. Expressions like LIMIT -1 must be explicitly disabled.

Copy link
Member

Choose a reason for hiding this comment

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

The simplest way is to check field type explicitly and process different cases for UInt64, Int64, Float64.
For Float64 we should also check that the limit is a whole number.

@@ -0,0 +1,7 @@
SELECT * FROM numbers(10) LIMIT 0.33 / 0.165 - 0.33 + 0.67;
SELECT * FROM numbers(10) LIMIT LENGTH('NNN') + COS(0), TO_DATE('0000-00-02');
Copy link
Member

@alexey-milovidov alexey-milovidov Feb 3, 2019

Choose a reason for hiding this comment

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

Date and DateTime should not work. Only numbers. We should check the data type.

SELECT * FROM numbers(10) LIMIT a + 5 - a;
SELECT * FROM numbers(10) LIMIT a + b;
SELECT * FROM numbers(10) LIMIT "Hello";
SELECT * FROM numbers(10) LIMIT ;
Copy link
Member

Choose a reason for hiding this comment

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

The cases of LIMIT BY and TOP should be also covered.

@alexey-milovidov
Copy link
Member

alexey-milovidov commented Feb 3, 2019

Нужно запустить сервер - он у меня запускается, но с какими-то подозрительными сообщениями: http://bit.ly/2UEHrCk

Судя по выводу, всё Ок.
Include not found - сообщение про подстановки в конфигурационном файле config.xml, которые могут загружаться из других файлов, но других файлов нет.

Нужно скопировать клиент в /usr/bin, но на маке, кажется, нельзя сделать это без специальных ухищрений а-ля запуск ОС в диагностическом режиме. Наверняка есть какой-то workaround на эту тему =)

Это не обязательно. Достаточно передать путь к клиенту в аргументах при запуске clickhouse-test.

Соответственно, тесты падают уже на 9-м (http://bit.ly/2UCwIbC), причём опция -k (продолжать, пока не сфейлится N тестов) почему-то не действует...

Последний тест "with server" запускает все функциональные тесты, которые также можно запустить с помощью clickhouse-test.

clickhouse-test можно запустить вручную, смотрите dbms/tests/

@k3box
Copy link
Author

k3box commented Feb 10, 2019

Dear ClickHouse Team,
I apologise for the long lasting delay with fixing review issues.
I hope I will tackle them in coming days

@alexey-milovidov
Copy link
Member

It doesn't work at all:

SELECT number FROM numbers(10) LIMIT 0 + 1

alexey-milovidov added a commit that referenced this pull request Feb 10, 2019
alexey-milovidov added a commit that referenced this pull request Feb 10, 2019
@alexey-milovidov alexey-milovidov merged commit 75c087b into ClickHouse:master Feb 10, 2019
@alexey-milovidov
Copy link
Member

Merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants