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

Fix perf-test input data and refactor two tests #4301

Merged
merged 6 commits into from
Feb 11, 2019

Conversation

alesapin
Copy link
Member

@alesapin alesapin commented Feb 7, 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):

  • Build/Testing/Packaging Improvement

Short description (up to few sentences):

  1. Add ability to send queries INSERT INTO tbl VALUES (.... to server without splitting on 'query' and 'data' parts.
  2. Fix ipv4 and ipv6 performance tests.

@alesapin
Copy link
Member Author

alesapin commented Feb 8, 2019

Unfortunately I had to modify ASTInsertQuery outside of parser https://github.com/yandex/ClickHouse/pull/4301/files#diff-387f58c9305eb5dcc6cdb110eaf1382fR172. Is it OK, or it should be avoided?

@alexey-milovidov
Copy link
Member

It's Ok.

if (!processSingleQuery(str, ast) && !ignore_error)
auto ast_to_process = ast;
if (insert && insert->data)
ast_to_process = nullptr;
Copy link
Member

Choose a reason for hiding this comment

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

Does it mean, that the client will send data in source format uncompressed?

Copy link
Member Author

@alesapin alesapin Feb 10, 2019

Choose a reason for hiding this comment

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

This code contained an error. If client were used in multiquery mode, than all queries after insert

insert into tbl values (1, 2, 3);
select * from system.numbers limit 10;
...etc...

were placed into insert->data field. But it didn't lead to problems, because server had ignored data filed of insert queries. Now server sometimes parse insert->data, so we have to process multiquery insert queries correctly on client side.
I don't know how this is connected with compression.

Copy link
Member

Choose a reason for hiding this comment

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

Queries were always sent to server without embedded data.

Copy link
Member

Choose a reason for hiding this comment

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

When data is sent in blocks (not embedded in query), it is compressed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Queries were always sent to server without embedded data.
This is true just for insert into ... values queries of clickhouse-client https://github.com/yandex/ClickHouse/blob/master/dbms/programs/client/Client.cpp#L883-L885. If there were insert-select query, more precisely buggy insert .. values (1, 2, 3); select * from ... query parsed with multiquery option, it was sent with embedded query->data, but data was always ignored on server side: https://github.com/yandex/ClickHouse/blob/master./dbms/src/Interpreters/executeQuery.cpp#L168-L169.

Since I add logic for embedded data processing on server side it became a problem, so I split parsing of insert queries (parse each query separately, not all of them together) if they are insert queries and contain embedded data.

Nothing have changed, native client (clickhouse-client) will send insert queries with not embedded data in compressed blocks. I still do not understand how specified line connected with compression of data. It just query parsing and there was bug for multiqueries.

@alexey-milovidov alexey-milovidov added the st-need-info We need extra data to continue (waiting for response) label Feb 9, 2019
@alesapin
Copy link
Member Author

Performance test failure is expected.

@alexey-milovidov alexey-milovidov merged commit 81a184c into master Feb 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
st-need-info We need extra data to continue (waiting for response)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants