-
Notifications
You must be signed in to change notification settings - Fork 2
Apply session_settings to every query
#65
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
Conversation
918c1c1 to
3721f7e
Compare
3721f7e to
2dcfa45
Compare
2dcfa45 to
1601abc
Compare
|
Nice refactor. Few things:
Nits:
Rest looks good, especially the empty string fix and using curl's URL API. |
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
|
Fixed @iskakaushik's issues in b015f06 (diff). |
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