Parameters in clickhouse-cpp

[OlegGalizin]

• params
• Nullable params

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

---

Oleg Galizin seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[OlegGalizin]

What can I do to help you to merge the request?

[OlegGalizin]

Is I doing something wrong or nobody has time to merge the request?

[1261385937]

@OlegGalizin
Please sign sign our Contributor License Agreement
And we will review your code.

[Enmk]

Proper high-level description of the added feature is missing. Please add one as a PR description. Having one greatly simplifies review.

Also, could you please create some unit-tests for this functionality? That way there is a chance that nobody is going to break this feature after it is merged. You can add test cases to the ut/client_ut.cpp or create a new test unit-test file for parameters specifically.

[Enmk]

Hi @OlegGalizin thanks for submitting a PR, please address mentioned issues.

[OlegGalizin]

I've add ci test. I did'n found how can I change a PR description.
The description is
Support query with parameters in clickhouse-cpp

If You can please add the description in proper place please.

[OlegGalizin]

Please help me run the tests

[Enmk]

Why not std::find_first_of ?

[OlegGalizin]

it more comfortable for me.
I think std::find_first_of needs
one extra compare because it return 'end' on failure
Or after call we needs compare with 'end' that worse then comparing with 0

[Enmk]

Please use functions from standard library whenever possible, instead of inventing new ones with 99% overlapping functionality. "comfortable for me" is not a valid excuse...

[OlegGalizin]

What about extra compare instruction and slow down the solution?
I must be sure that you are understand that you suggest

se for example find_first_symbols_sse2 in main clickhouse repo that also uses double loop and return nullptr

[Enmk]

First, find_first_symbols_sse2 was introduced to be used in a very hot pieces of code, which I believe is not the case here.
Second, there is a very high chance that compiler can optimize call std::find_first_of (e.g. using SSE) way beyond anything you are trying to gain here.
And last, find_first_symbols_sse2 supports two modes: "return nullptr" and "return end" 😉.

This looks like you are trying to prematurely optimize O(N + 1) into O(N), by introducing something that would unnecessary complicate the support.

[Enmk]

Please fix issues in the PR

[Enmk]

Please fix the tests

[Enmk]

@OlegGalizin please fix tests

[Enmk]

@OlegGalizin ping, Client/ClientCase.QueryParameters is failing on almost every platform...
BTW, to simplify build-test-review loop, you can fix test using your own fork of clickhouse-ccp. Runners are provided by github, so it would work on your repo.

[OlegGalizin]

Client/ClientCase.QueryParameters is failing on almost every platform

@Enmk
I see. Clickhouse server doesn't support parameters in the test system.

+-----------+
| version() |
+-----------+
|    String |
+-----------+
| 22.3.20.29 |
+-----------+

I checked
ClickHouse 24.7.1.2195 (revision: 54488, git hash: 452d463d77eed30aab18c77933e68316f8c57295
To work with parameters revision of ClickHouse server must be >= 54459

Do you want to switch off the test?
Will you update the test environment?

[Enmk]

Client/ClientCase.QueryParameters is failing on almost every platform

@Enmk I see. Clickhouse server doesn't support parameters in the test system.

+-----------+
| version() |
+-----------+
|    String |
+-----------+
| 22.3.20.29 |
+-----------+

I checked ClickHouse 24.7.1.2195 (revision: 54488, git hash: 452d463d77eed30aab18c77933e68316f8c57295 To work with parameters revision of ClickHouse server must be >= 54459

Do you want to switch off the test? Will you update the test environment?

Some unit tests are conditionally skipped based on clickhouse version, please check the code and modify your tests accordingly.

And please either get rid of find_quoted_chars or provide a benchmark that shows at least 10% smaller time against std::find_first_of. (e.g. using https://quick-bench.com)

[OlegGalizin]

please either get rid of find_quoted_chars or provide a benchmark

Please do it himself.

[OlegGalizin]

How about merge the PR to master?