Add support for raw queries without going through query builder (#234) by ozgurcancal · Pull Request #243 · ClickHouse/clickhouse-rs

ozgurcancal · 2025-06-18T22:38:12Z

Summary

Implements raw SQL query support as requested in #234.

CLAassistant · 2025-06-18T22:38:18Z

All committers have signed the CLA.

slvrtrn · 2025-06-19T12:47:27Z

+    /// Question marks are escaped using odd/even strategy:
+    /// - `?` (odd count) → `??`
+    /// - `??` (even count) → `??` (unchanged)
+    /// - `???` (odd count) → `????`


I wonder, if this is a raw query without support to binds on ?, why it is needed? For example, this is how the CLI client behaves:

SELECT '??', '?', '????' ┌─'??'─┬─'?'─┬─'????'─┐ 1. │ ?? │ ? │ ???? │ └──────┴─────┴────────┘

i.e. no additional translation for ? symbols. Or is it because of some HTTP interface quirks?

Thanks for your comment. My tests show that Literal ? in SQL is escaped as ?? for ClickHouse HTTP interface whereas CLI client sends SQL directly so it is an HTTP interface quirk as you sad

I think that might be a URL encoding oddity, then. It is worth checking that it is performed correctly.

For example, using POST:

curl -XPOST "http://localhost:8123" --data-binary "select '?', '??', '???' FORMAT JSONEachRow" {"'?'":"?","'??'":"??","'???'":"???"}

GET with proper URI encoding:

curl "http://localhost:8123?query=SELECT%20'%3F'%2C'%3F%3F'%2C'%3F%3F%3F'%20FORMAT%20JSONEachRow" {"'?'":"?","'??'":"??","'???'":"???"}

Where the query is encoded as:

encodeURIComponent("SELECT '?','??','???' FORMAT JSONEachRow") "SELECT%20'%3F'%2C'%3F%3F'%2C'%3F%3F%3F'%20FORMAT%20JSONEachRow"

All in all, that could be related to #230 and this bit in particular:

https://github.com/ClickHouse/clickhouse-rs/blob/main/src/query.rs#L158-L169

Thanks I think my last commit would fix the issue, I also compared the output with the CLI client.

slvrtrn · 2025-06-19T12:49:12Z

+    /// // query.bind(value)  ❌
+    /// // query.param("key", value)  ❌
+    /// ```
+    pub fn query_raw(&self, query: &str) -> query_raw::QueryRaw {


I am curious if we can just add a builder method to the main Query that disables query builder usage, as well as some bind checks.

Thanks for the comment, I just wanted to implement a solution with zero change on the existing codebase to make further changes and tests easier. I could adjust builder mechanism using a bool flag too such as;

pub struct Query {
client: Client,
sql: SqlBuilder,
raw: bool
}

@loyd @serprex, what is your opinion on this? Shall we just keep a single Query with a builder method and a few additional internal boolean checks, or keep a QueryRaw with (almost) similar API?

would have to compare the two choices to really decide, ideally you have Query/QueryRaw & implement Query on top of QueryRaw, but very likely that runs into some hurdle

slvrtrn · 2025-06-19T12:49:32Z

+    #[track_caller]
+    pub fn param(self, _name: &str, _value: impl Serialize) -> Self {
+        panic!("cannot set parameters on raw query - use regular query() for parameter binding");
+    }


if this is a separate struct/impl, why do we even need these methods that always panic?

Thanks for the detailed review. I thought the developer should be notified about the misuse of the functionality to help prevent future bugs.

slvrtrn

Thanks for the contribution. I like that there is a lot of tests and docs. I have a few questions about the implementation details, though, see the comments.

loyd · 2025-06-21T14:26:03Z

+async fn query_raw_preserves_exact_sql() {
+    let client = prepare_database!();
+
+    // Test that raw query preserves the exact SQL including whitespace and formatting


It can check, for instance, system.query_log to ensure it, but currently it doesn't ensure this statement

Thanks for the comment, it now compares with the system logs

loyd · 2025-06-21T14:33:22Z

    /// [`Identifier`]: crate::sql::Identifier
    #[track_caller]
    pub fn bind(mut self, value: impl Bind) -> Self {
+        if self.raw {


In general, I'm fine with this implementation.

However, it's slightly strange to check in runtime properties that can easily be checked at compile time:

struct Query<M = DefaultMode> { .. } impl Query<DefaultMode> { pub(crate) fn new(..) -> Self { .. } pub fn bind(..) { .. } // only in the default mode } impl Query<RawMode> { pub(crate) fn new(..) -> Self { .. } }

and, obviously, a trait (sealed) to turn bind_fields on/off

anyway up to @slvrtrn

Oh, it's probably duplicate of #243 (comment)

We could also try to explore an option of using QueryRaw as a building block for Query, as @serprex suggested. But perhaps current impl is good enough to start with, so let's not change it.

loyd · 2025-06-21T14:42:34Z

        query::Query::new(self, query)
    }

+    /// Starts a new SELECT/DDL query that will be used as-is without any processing.


I think we should provide more documentation on this difference to help newcomers choose the right constructor

slvrtrn · 2025-06-23T09:07:07Z

@ozgurcancal, let's resolve the conflicts, add a bit more docs as @loyd requested and then we can merge.

- Update documentation and tests

slvrtrn · 2025-06-26T09:25:34Z


- [usage.rs](usage.rs) - creating tables, executing other DDLs, inserting the data, and selecting it back. Optional cargo features: `inserter`.
+- [usage.rs](usage.rs) - creating tables, executing other DDLs, inserting the data, and selecting it back. Additionally, it covers `WATCH` queries. Optional cargo features: `inserter`, `watch`.
+- [query_raw.rs](query_raw.rs) - raw queries without parameter binding, with question mark escaping. FORMAT is the RowBinary by default


WATCH is removed as of #245, since it is deprecated feature, and its implementation was incompatible with CH 25+.

with question mark escaping

is it still true?

slvrtrn · 2025-06-26T09:28:16Z

    /// [`Identifier`]: crate::sql::Identifier
    #[track_caller]
    pub fn bind(mut self, value: impl Bind) -> Self {
+        if self.raw {


We could also try to explore an option of using QueryRaw as a building block for Query, as @serprex suggested. But perhaps current impl is good enough to start with, so let's not change it.

slvrtrn · 2025-06-26T11:38:44Z

+
+    // Test question marks in SQL comments - should work without binding
+    let result = client
+        .query_raw("SELECT 1 /* What? How?? Why??? */ WHERE 1=1")


I like this test, very nice 👍

#243 (comment)
It would be great, but I think the new builder will share a lot with the current one, so we’ll probably need a common base trait for both, and that means a bunch of changes.

slvrtrn · 2025-06-26T11:51:45Z

CH Cloud test failures can be ignored, cause it's secrets pull issue for now, need to re-do the setup to support PRs from external contributors.
The rest (rustfmt, docs) should be fixed before merging.

ozgurcancal · 2025-06-26T15:00:47Z

CH Cloud test failures can be ignored, cause it's secrets pull issue for now, need to re-do the setup to support PRs from external contributors. The rest (rustfmt, docs) should be fixed before merging.

I have addressed the suggested fixes. Thanks for the support, next time my PR will be much cleaner :)

loyd · 2025-07-02T07:25:15Z

I think we shouldn't merge this PR until #157 (comment) is discussed

ozgurcancal mentioned this pull request Jun 18, 2025

Add support for raw queries without going through query builder #234

Open

slvrtrn reviewed Jun 19, 2025

View reviewed changes

slvrtrn suggested changes Jun 19, 2025

View reviewed changes

ozgurcancal requested a review from slvrtrn June 20, 2025 17:45

ozgurcancal force-pushed the development branch 3 times, most recently from 805acf0 to 0c070fe Compare June 20, 2025 21:41

ozgurcancal closed this Jun 20, 2025

ozgurcancal force-pushed the development branch from 0c070fe to 54a80a0 Compare June 20, 2025 21:46

ozgurcancal reopened this Jun 20, 2025

loyd reviewed Jun 21, 2025

View reviewed changes

loyd previously approved these changes Jun 21, 2025

View reviewed changes

slvrtrn previously approved these changes Jun 23, 2025

View reviewed changes

ozgurcancal dismissed stale reviews from slvrtrn and loyd via d725f3c June 25, 2025 20:06

Add query_raw method for raw SQL execution

39e36b9

- Update documentation and tests

ozgurcancal force-pushed the development branch from d725f3c to 39e36b9 Compare June 25, 2025 21:14

slvrtrn previously approved these changes Jun 26, 2025

View reviewed changes

Add test to verify logged SQL matches executed statement

b12ad8f

ozgurcancal dismissed slvrtrn’s stale review via b12ad8f June 26, 2025 14:20

ozgurcancal force-pushed the development branch from 3b1ee71 to b12ad8f Compare June 26, 2025 14:20

Merge branch 'ClickHouse:main' into development

6bfcf50

docs: improve documentation for query_raw method

2a3e0f8

ozgurcancal force-pushed the development branch from fdeab30 to 2a3e0f8 Compare June 26, 2025 16:01

ozgurcancal mentioned this pull request Jun 26, 2025

Interpret ? as a parameter only if .bind was called at least once #157

Open

ozgurcancal requested a review from slvrtrn June 29, 2025 17:56

slvrtrn requested a review from abonander September 18, 2025 09:52

Uh oh!

Conversation

ozgurcancal commented Jun 18, 2025

Summary

Uh oh!

CLAassistant commented Jun 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ozgurcancal Jun 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slvrtrn Jun 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ozgurcancal Jun 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slvrtrn Jun 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ozgurcancal Jun 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slvrtrn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

loyd Jun 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slvrtrn Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slvrtrn commented Jun 23, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slvrtrn Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slvrtrn commented Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ozgurcancal commented Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

loyd commented Jul 2, 2025

Uh oh!

Reviewers

Assignees

Labels

CLAassistant commented Jun 18, 2025 •

edited

Loading

ozgurcancal Jun 19, 2025 •

edited

Loading

slvrtrn Jun 19, 2025 •

edited

Loading

ozgurcancal Jun 19, 2025 •

edited

Loading

slvrtrn Jun 19, 2025 •

edited

Loading

ozgurcancal Jun 19, 2025 •

edited

Loading

loyd Jun 21, 2025 •

edited

Loading

slvrtrn Jun 26, 2025 •

edited

Loading

slvrtrn Jun 26, 2025 •

edited

Loading

slvrtrn commented Jun 26, 2025 •

edited

Loading

ozgurcancal commented Jun 26, 2025 •

edited

Loading