Conversation
918c1c1 to
3721f7e
Compare
3721f7e to
2dcfa45
Compare
serprex
approved these changes
Dec 4, 2025
2dcfa45 to
1601abc
Compare
Collaborator
|
Nice refactor. Few things:
Nits:
Rest looks good, especially the empty string fix and using curl's URL API. |
Collaborator
Author
Makes sense.
It shouldn't need to
Check.
Thanks! |
Add a new struct, `ch_query`, and replace all instances of passing a
`char *` with passing `ch_query`. This allows it to included additional
fields relevant to the query, starting here with `sql` and `settings`,
the latter currently being a PostgreSQL `List` of the settings parsed
from `pg_clickhouse.session_settings`. Provide `new_query()` to keep its
construction in one place; in the future it might take parameters to
merge additional settings fetched from a server or user mapping to merge
into the GUC's settings.
Update `ch_binary_simple_query()` to iterate over the list of settings
in the `ch_query` and apply them to the query as a `QuerySettings` map.
Add a comment to do the same for inserts, held until
[clickhouse-cpp#453] is merged and released.
Update `ch_http_simple_query()` to use `curl_url_set()` to build each
request URL, starting with the base URL and query ID, then adding
another query parameter for each item in `ch_query.settings`. This also
eliminates the need for `ch_http_connection_t.base_url_len`.
Add tests to ensure consistent application of a variety of settings via
both the binary and http engines.
While at it, fix a bug where http query results treated an empty string
as `NULL` and add a test for it. Also replace a buffer allocation with
simple use of `psprintf()`, which is automatically cleaned up in the
current PostgreSQL memory context.
Other bits:
* Document `pg_clickhouse.session_settings`
* Rename `connect.h` to `engine.h`, which better describes its purpose
providing objects and functions used by both the http and binary
engines
* Remove the unused `cursor_free_method` field from the
`libclickhouse_methods` struct
[clickhouse-cpp#453]: ClickHouse/clickhouse-cpp#453
1601abc to
b015f06
Compare
Collaborator
Author
|
Fixed @iskakaushik's issues in b015f06 (diff). |
iskakaushik
approved these changes
Dec 5, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add a new struct,
ch_query, and replace all instances of passing achar *with passingch_query. This allows it to included additional fields relevant to the query, starting here withsqlandsettings, the latter currently being a PostgreSQLListof the settings parsed frompg_clickhouse.session_settings. Providenew_query()to keep its construction in one place; in the future it might take parameters to merge additional settings fetched from a server or user mapping to merge into the GUC's settings.Update
ch_binary_simple_query()to iterate over the list of settings in thech_queryand apply them to the query as aQuerySettingsmap. Add a comment to do the same for inserts, held until clickhouse-cpp#453 is merged and released.Update
ch_http_simple_query()to usecurl_url_set()to build each request URL, starting with the base URL and query ID, then adding another query parameter for each item inch_query.settings. This also eliminates the need forch_http_connection_t.base_url_len.Add tests to ensure consistent application of a variety of settings via both the binary and http engines.
While at it, fix a bug where http query results treated an empty string as
NULLand add a test for it. Also replace a buffer allocation with simple use ofpsprintf(), which is automatically cleaned up in the current PostgreSQL memory context.Other bits:
pg_clickhouse.session_settingsconnect.htoengine.h, which better describes its purpose providing objects and functions used by both the http and binary enginescursor_free_methodfield from thelibclickhouse_methodsstruct